Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Improve symmetry for CXXFLAGS and LDFLAGS #791

Closed
wants to merge 5 commits into from

Conversation

webmaster128
Copy link
Collaborator

@webmaster128 webmaster128 commented Jan 2, 2017

For cross-compiling for Android, I need to set additional LDFLAGS and CXXFLAGS (which are not "abi flags").

Before, LDFLAGS could only be set at make time and CXXFLAGS could only be set at configure time via --cc-abi-flags.

This PR allows using the LDFLAGS/CXXFLAGS environment variable at make time. It would be possible to move those to configure time as well, if desired.

I think this supersedes the --cc-abi-flags configure time argument, which tends to be a messy place to hack in all kind of compile flags. Is there a good reason to preserve it?

@webmaster128
Copy link
Collaborator Author

Updated the initial PR, which is working for me now. The remaining CI failures are related to #792.

Simon Warta added 5 commits January 3, 2017 00:25
mach_abi_link_flags() is only osed to set %{cxx_abi_flags}, which does
not need an extra leading space.
The result of mach_abi_flags() is added to the compile and link command.
@codecov-io
Copy link

codecov-io commented Jan 3, 2017

Current coverage is 90.06% (diff: 100%)

No coverage report found for master at c2a723f.

Powered by Codecov. Last update c2a723f...4e6d47e

@webmaster128
Copy link
Collaborator Author

I believe the environment variables LDFLAGS/CXXFLAGS should be read at configure time and then be written into the Makefile. This is how cmake and autoconf and most other build systems I know work. It has the advantage that all configuration is done in one step and all flags are documented in the resulting Makefile.

Moving LDFLAGS from make time to configure time break build setups that work like

./configure.py ...
LDFLAGS=xy make

instead of

export LDFLAGS=xy
./configure.py ...
make

Is it okay to perform this incompatible change?

@randombit
Copy link
Owner

@webmaster128 Reading the env variables at configure time seems like the right decision to me, for the reasons you outline. It is OK to perform this change, we should just make sure there is a clear release note on it. Do you want to do this change as part of this PR?

@webmaster128
Copy link
Collaborator Author

Thanks for the feedback. I'll rework this one entirely after different things became clear to me.

I am not sure about one thing: is there a difference between CXXFLAGS and what we call --cc-abi-flags? Should both be used for every call of CXX, for compiling and linking?

@webmaster128 webmaster128 changed the title Improve symmetry for CXXFLAGS and LDFLAGS [WIP] Improve symmetry for CXXFLAGS and LDFLAGS Jan 3, 2017
@randombit
Copy link
Owner

I think classically CXXFLAGS is only used for individual file compilation, whereas --cc-abi-flags original meaning is specifically for flags which affect ABI or which otherwise must be used at compile time, shared object link time, and exe link time (for example -m64, -fstack-protector, or -fsanitize=address). It just gets abused a lot to pass extra flags that should really be CXXFLAGS as they only affect compilation of individual source files.

In your proposal, since configure is already processing the information and exporting it into the makefile templates, it should probably also take --cxxflags= and --ldflags= options (better name? i am not sure if autoconf exposes this via a flag) which can be used instead of (or to override) settings in env variables.

@webmaster128
Copy link
Collaborator Author

webmaster128 commented Jan 3, 2017

it should probably also take --cxxflags= and --ldflags= options (better name? i am not sure if autoconf exposes this via a flag) which can be used instead of (or to override) settings in env variables.

I guess it is okay to provide just one way (for simplicity), and I prefer --cxxflags/--ldflags over CXXFLAGS/LDFLAGS because we use arguments for everything else.

It just gets abused a lot to pass extra flags that should really be CXXFLAGS as they only affect compilation of individual source files.

Hmm... cmake passes everything from CXXFLAGS to the link call as well: running CXXFLAGS=-Dheyho12 cmake .. results in the link command

./test/CMakeFiles/smartsqlite_tests.dir/link.txt:/usr/lib/ccache/c++ -Dheyho12 -std=c++11 -pthread -Wall -Wextra -Wpedantic -Wcast-qual -Wformat=2 -Wlogical-op -Wmissing-include-dirs -Wswitch-default -Wno-maybe-uninitialized -Woverloaded-virtual CMakeFiles/smartsqlite_tests.dir/blob_test.cpp.o CMakeFiles/smartsqlite_tests.dir/connection_test.cpp.o CMakeFiles/smartsqlite_tests.dir/logging_test.cpp.o CMakeFiles/smartsqlite_tests.dir/nullable_test.cpp.o CMakeFiles/smartsqlite_tests.dir/scopedsavepoint_test.cpp.o CMakeFiles/smartsqlite_tests.dir/scopedtransaction_test.cpp.o CMakeFiles/smartsqlite_tests.dir/statement_test.cpp.o CMakeFiles/smartsqlite_tests.dir/version_test.cpp.o -o smartsqlite_tests -rdynamic ../src/libsmartsqlite.a ../lib/gmock/libgmock.a ../lib/gmock/libgmock_main.a -ldl

And Gentoo puts ABI information into CXXFLAGS, that are definitely probably required at link time.

So maybe --cxxflags can replace --cc-abi-flags.

@webmaster128
Copy link
Collaborator Author

Closing in favour of #1237.

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.

None yet

3 participants