pyroscope / rtorrent-ps

:art: Extended rTorrent distribution with a fully customizable canvas and colors, other feature additions, and complete docs.
http://rtorrent-ps.readthedocs.io/
GNU General Public License v2.0
465 stars 43 forks source link

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

Open zetafunction opened 5 years ago

zetafunction commented 5 years ago

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/rtorrent-ps#76.

zetafunction commented 5 years ago

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:

chros73 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.