Skip to content

Fix CXXFLAGS and only set the C++ version in CXXFLAGS.#118

Open
zetafunction wants to merge 1 commit into
pyroscope:masterfrom
zetafunction:cxxflags
Open

Fix CXXFLAGS and only set the C++ version in CXXFLAGS.#118
zetafunction wants to merge 1 commit into
pyroscope:masterfrom
zetafunction:cxxflags

Conversation

@zetafunction
Copy link
Copy Markdown

CPPFLAGS are used for building both C and C++ programs, and clang
complains when -std=c++11 is used when the driver is invoked in C mode.
Using CXXFLAGS to set this allows rtorrent-ps to be successfully built
with clang.

Unfortunately, fixing CXXFLAGS requires a new hack to ensure -O2 is
uniformly used. Otherwise, autoconf will break when detecting XMLRPC-C
support with an error like the one in #76.

CPPFLAGS are used for building both C and C++ programs, and clang
complains when -std=c++11 is used when the driver is invoked in C mode.
Using CXXFLAGS to set this allows rtorrent-ps to be successfully built
with clang.

Unfortunately, fixing CXXFLAGS requires a new hack to ensure -O2 is
uniformly used. Otherwise, autoconf will break when detecting XMLRPC-C
support with an error like the one in pyroscope#76.
@zetafunction
Copy link
Copy Markdown
Author

Would you be OK with this patch to clean up build.sh a little? Touching build config is always a bit risky, since it could break on a platform that didn't get tested: I did test that all builds successfully with the default build command with no additional customizations, and that it also builds with CC/CXX set to clang/clang++.

I understand that clang is not a supported compiler. However, following the normal conventions for CPPFLAGS and not specifying C++-only flags in CPPFLAGS seems better for compatibility, and should be better in the long-run.

Some investigation notes:

  • I noticed that -O2 was being implicitly passed when CXXFLAGS was unset. I'm not familiar enough with autoconf to understand where the default value of -g -O2 is coming from.
  • If -O2 is not passed in the compile command for the XMLRPC-C test program, the build fails due to unresolved references like in Build error on arm32v7/debian:stretch #76
  • The simplest "fix" is to just make sure CFLAGS/CXXFLAGS always includes -O2 for now. I picked -g -O2 for default CFLAGS/CXXFLAGS values, since that's what autoconf was already selecting.
  • However, it's a hack, since the right fix is to not link libtorrent into the autoconf test program. But I haven't figured out where that's coming from yet. I'll poke at this more and see if I can figure out where this is coming from.

@chros73
Copy link
Copy Markdown
Contributor

chros73 commented Jan 13, 2019

I noticed that -O2 was being implicitly passed when CXXFLAGS was unset. I'm not familiar enough with autoconf to understand where the default value of -g -O2 is coming from.

In case of 0.9.6/0.13.6 the -O2 flag comes from rtorrent/libtorrent. This behavior has been changed since 0.9.7/0.13.7.

Normally, the building environment has to take care of the basic flags if they are not set. If they are then they can override those default flags (at least with certain distributions). Hence the check was added in 0.9.7/0.13.7.

What distribution do you use?

@zetafunction
Copy link
Copy Markdown
Author

I've been testing on debian stretch, building both 0.9.6/0.13.6 and 0.9.7/0.13.7 (I have a set of local patches to make the latter work).

@chros73
Copy link
Copy Markdown
Contributor

chros73 commented Jan 13, 2019

and 0.9.7/0.13.7 (I have a set of local patches to make the latter work)

You don't have to do this: rtorrent-ps-ch
The build script is a bit different than pyroscope's.

@zetafunction
Copy link
Copy Markdown
Author

zetafunction commented Jan 13, 2019

Maybe, but making the minor build.sh tweaks still seems like a step in the right direction. For example, it could have possibly helped with rakshasa/rtorrent#300 (and the associated dupes).

As mentioned, the 'proper' fix is to still make sure the XMLRPC-C test program doesn't build with -ltorrent and only add -ltorrent into the linker flags for rtorrent itself. That's something I'm investigating still though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants