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

Apply *_FOR_BUILD flags to the native build if defined #1488

Closed

Conversation

fabiangreffrath
Copy link
Contributor

If no value is given, CMake will unset the variables just as before.

If no value is given, CMake will unset the variables just as before.
Copy link
Member

@derselbst derselbst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, I still don't think it's worth to incorporate such a change. Also, it doesn't work. Given the way the gentables project is currently called, no value will ever be given for any of the *_FOR_BUILD vars. So this would just always behave as complicated unset().

@helmutg
Copy link

helmutg commented Feb 24, 2025

Frankly, I still don't think it's worth to incorporate such a change. Also, it doesn't work. Given the way the gentables project is currently called, no value will ever be given for any of the *_FOR_BUILD vars. So this would just always behave as complicated unset().

It is a user (e.g. the Debian distribution) that sets *_FOR_BUILD. The origin of this PR is that Debian's QA systems verify that desired compiler flags make it into the build logs. In this instance, the flags are not strictly required, but it looks simpler to supply them than to figure out an exception rule for those invocations that don't technically need them.

And then you figured that this change does not regress anything and works as before (except when Debian uses it). Isn't that good enough then?

@derselbst
Copy link
Member

It is a user (e.g. the Debian distribution) that sets *_FOR_BUILD

I got that, yes. But still, I don't see how this can work via ExternalProject. Have you ever tested this? If so, pls. show me how you "set" those variables so that they're actually propagated to the gentables project.

@fabiangreffrath
Copy link
Contributor Author

They are environment variables, not CMake variables.

@derselbst
Copy link
Member

They are environment variables, not CMake variables.

*_FOR_BUILD are CMake Variables. The following two examples should fail to compile:

cmake -DCC_FOR_BUILD=/usr/bin/false  -DCFLAGS_FOR_BUILD=gehtnich ..
export CC_FOR_BUILD=/usr/bin/false
export CFLAGS_FOR_BUILD=gehtnich
cmake ..

But they both compile fine. Why? Because you proposed a change, that doesn't work and that has presumably never been tested. Pls. come back with a proposal that actually works.

PS: I would accept one that matches up more closely with the current approach to specify a host compiler.

@derselbst derselbst closed this Feb 24, 2025
@fabiangreffrath
Copy link
Contributor Author

You are right, sorry. I misquoted the variables to read them from the environment.

With the following patch it will work if the environment variables are set during the build (i.e. ninja or make) phase, not the configure (i.e. CMake) phase.

However, I understand if you don't want to hear about this topic anymore. Sorry for wasting your time. :/

--- a/src/gentables/CMakeLists.txt
+++ b/src/gentables/CMakeLists.txt
@@ -2,13 +2,13 @@ cmake_minimum_required(VERSION 3.5)
 
 # remove $CC from the current environment and by that force cmake to look for a (working) C compiler,
 # which hopefully will be the host compiler
-unset(ENV{CC})
+set(ENV{CC} $ENV{CC_FOR_BUILD})
 
 # also unset $CFLAGS to avoid passing any cross compilation flags to the host compiler
-unset(ENV{CFLAGS})
+set(ENV{CFLAGS} $ENV{CFLAGS_FOR_BUILD} $ENV{CPPFLAGS_FOR_BUILD})
 
 # linker flags as well
-unset(ENV{LDFLAGS})
+set(ENV{LDFLAGS} $ENV{LDFLAGS_FOR_BUILD})
 
 project (gentables C)
 

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.

3 participants