Get rid of default command line flags in Makefile about cctz HOT 16 CLOSED

Hi Alfredo. I'm happy to change the default. If you want environment variables to take precedence however, I would suggest the -e (--environment-overrides) make flag.

from cctz.

@devbww yep, it's possible to force the flags using -e but this isn't recommended practice (see here). It's recommended to instead use conditional setting of variables eg; CXXFLAGS ?= -O3. This allows a default build to use sensible release flags and if a user wants something special it'll get pulled in from the enviroment variables without explicitly overriding the makefile variable.

I think a sensible default is -O3 since from the looks of it cctz doesn't seem to rely on undefined behavior (we've been running it in production over a year that way). Keeping a separate STD variable is smart since some older compilers might use -std=c++0x.

It doesn't make sense to include debug flags or march=native flags by default since these adversely affect optimization and portability respectively. A user can already easily get a debug build by CXXFLAGS="-O0 -g" make or a native build by CXXFLAGS="-O3 -march=native" make.

The compiler also should definitely not be specified by default.

Thoughts?

from cctz.

Sounds good to me. Send a diff for what you'd like to see. Thanks.

from cctz.

Hi @devbww see #35

from cctz.

Hi @iamthebot, @devbww,

I just had a quick look on your patch and the resulting Makefile.

The default build is not creating debug symbols (CXXFLAGS option '-g'). Since a build with '-g' has identical code but added symbol information, building always with '-g' does no harm. If debug symbols are really not required, the 'strip' command or the 'install -s' option can easily remove them on the fly while installing, if wanted. Yet, in the local build tree executables remain with full debug information for the otherwise identical release binary, so that crashes can still be resolved post mortem by pointing debug tools to the object images with debug symbols.

What is $(TEST_FLAGS) for? If CXXFLAGS can be set by the caller (as for "-O3"), then TEST_FLAGS can be set in CXXFLAGS of the calling instance, too.

The Makefile defines both CPPFLAGS and CXXFLAGS for the implicit build rule. CPPFLAGS are for the preprocessor, CXXFLAGS apply to c++ files only (cf. https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html). CPPFLAGS work in the current set-up, since they are applied to almost everything including C++, plain C and FORTRAN files (probably not intended for flags like -std=c++11?).

As cctz is a c++-only project, it might be cleaner and more consistent to use CXXFLAGS exclusively, and append all required flags there. Also, LDFLAGS should use append-semantics for essential flags like -pthread, so that the caller can override it with flags like "-fsanitize=xxx":

CXXFLAGS ?= -O3
CXXFLAGS += -g -Wall -I$(SRC)include -std=$(STD) -pthread -fPIC -MMD
LDFLAGS += -pthread

This would build an optimized release by default. For a debug build, compilation is started with

$ CXXFLAGS="-O0 -ftrapv -fsanitize-undefined" LDFLAGS="${CXXFLAGS}" make
### or whatever flags are required to debug a particular problem

In an unrelated note, the use pattern of the $(SUDO) variable seems a bit esoteric and not needed, since you can always do "make && sudo make install". Is this pattern commonly used or expected in other projects or build environments I am possibly not aware of? If not, the Makefile could get further simplified by removing $(SUDO).

from cctz.

Additional note: '-OX' optimization flags should be set for LDFLAGS, too, as some link-time optimizations like -fmerge-constants, -freorder-functions, &c, are performed at link time:

CXXFLAGS ?= -O3
CXXFLAGS += -g -Wall -I$(SRC)include -std=$(STD) -pthread -fPIC -MMD
LDFLAGS ?= -O3
LDFLAGS += -pthread

from cctz.

@Siesh1oo I just wanted to submit a very minimal patch to fix the more pressing issue (that the default wasn't a release build at all). But now that you mention it, there are many other underlying issues with the Makefile.

Regarding TEST_FLAGS this, I believe was to provide future support for googletest. In my experience, if you want to use gtest it's better to use a more sophisticated build system (eg; CMake, Bazel, Autotools) that lets you pull the gtest dependency. I believe it's not recommended to use a shared gtest library anymore (in fact it's not available on many Linux distros). I think adding a CMakeLists.txt would be a separate issue though.

I'm removing TEST_FLAGS since it isn't even used.

As for the $(SUDO) pattern I've removed that. There's no good reason for it. If you don't have sudo installed on the machine you can just use su -c when installing. Tested the latest Makefile on Linux (RHEL 7.2) and FreeBSD 11.0.

@devbww PR #35 is updated with these changes.

from cctz.

What is the intent of "CC=$(CXX)"?

from cctz.

@Siesh1oo they're using an implicit build target which calls $(CC) by default. If you remove that line, it won't work without redoing the build targets in the Makefile. I'd rather have that be a separate patch. Would that work?

from cctz.

Hm, you refer to the linker rule? It might be cleaner to either add "-lstdc++" to LDFLAGS, or to override the implicit rule:

% : %.o
        $(CXX) $^ $(LDFLAGS) -o $@

from cctz.

@Siesh1oo setting $(CC) to $(CXX) is actually very common. There is an alternative formulation: setting LINK.o to LINK.cc that's perhaps more clear in its intention. I just made a commit to this effect.

Explicitly linking stdc++ is problematic because some build environments contain multiple versions of the standard library to link against with a nonstandard default rule.

from cctz.

Closed the previous PR due to an issue with one of the commits, #38 contains the necessary changes to make this compliant. @Siesh1oo please review if it's to your liking.

from cctz.

looks fine to me

from cctz.

Hi. Sorry I've been slow at looking at this (and other pending CCTZ issues). I'll try to catch up over the weekend. Thanks.

from cctz.

I believe I have committed most of what you requested.

I'd like to leave the TEST_* stuff if it makes no difference to anyone else as I use it for some extra testing (outside of bazel).

I omitted the .gitignore changes as I'd like to encourage the "-C build" usage. Do you not like that?

Anyway, take a look. Thanks.

from cctz.

@devbww looks good. Closing this and #38.

from cctz.