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

Always read from all CFLAGS-style flags #1401

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

madsmtm
Copy link
Collaborator

@madsmtm madsmtm commented Feb 14, 2025

Reading from just one of these means that users setting different environment variables in different parts of the build process would not get flags from the other parts.

This tripped me up in rust-lang/rust#133092 / rust-lang/rust#136984, where I thought that since I was extending CFLAGS_* instead of overwriting it, I was allowing cc-rs to also read from CFLAGS.

Instead, we now read all of the flags, but in order such that more specific flags override less specific ones.

So e.g. when users set CFLAGS=-flag1 CFLAGS_aarch64_apple_darwin=-flag2 cargo build, we end up passing -flag1 -flag2 to the compiler (in that order specifically).

NOTE: This has the slight chance of breaking builds where users assume that CFLAGS_$TARGET overwrites CFLAGS. I will argue that that is likely to be the minority, relative to users that do want flags from all CFLAGS* env vars.
To fix such cases, users should pass the negative flag in CFLAGS_$TARGET that they don't want CFLAGS to set (e.g. if CFLAGS contains -pthread, users should pass CFLAGS_$TARGET=-no-pthread).

Reading from just one of these means that users setting different
environment variables in different parts of the build process would not
get flags from the other parts.

Instead, we read all of the flags, but in a specific order such that
more specific flags override less specific ones.
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, I think this PR is sound, just have some code maintenance suggestion

@madsmtm madsmtm requested a review from NobodyXu February 14, 2025 13:24
@NobodyXu NobodyXu enabled auto-merge (squash) February 14, 2025 13:34
@NobodyXu NobodyXu merged commit 0dd683c into rust-lang:main Feb 14, 2025
73 checks passed
@madsmtm madsmtm deleted the all-cflags-from-env branch February 14, 2025 14:08
@madsmtm madsmtm mentioned this pull request Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants