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

Bugfix/crt static in config #6736

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ohadravid
Copy link

ohadravid commented Mar 12, 2019

I ran into an issue when compiling xz2 crate under Windows, where .cargo/config RUSTFLAGS does not set the correct env vars in the build (full details can be found at
alexcrichton/xz2-rs#46).

The problem seems to be that an empty env var will take precedence over the config file.

Cargo version is:
cargo 1.34.0-nightly (5c6aa46 2019-02-22)

I added a test for this and a small fix.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Mar 12, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 12, 2019

Thanks for this @ohadravid!

I'm gonna run this by others on @rust-lang/cargo since this is a beahvioral change, but I think this is a reasonable semantics for Cargo to implement. Cargo doesn't necessarily uniformly handle empty env vars though, so I'd be curious about others' thoughts on that

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link
Collaborator

rfcbot commented Mar 12, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Mar 12, 2019

So how do you override a config file and use the defaults after this change?

@ohadravid

This comment has been minimized.

Copy link
Author

ohadravid commented Mar 13, 2019

I can change the code so it's possible to differentiate between an empty string and some whitespace and treat whitespace as the 'default', but this would be very confusing for users (breaks the common expectations around environment variables).

Also as a user, I think of whatever's in the config file is the 'default'. If I want something else I'll probably grab whatever's in there and add something. For example I have a crate which is built against a static lzma.so file, and trying a 'default' (==empty) flags can break the build.

It's worth noting that the current behavior for the 'build.rs' file is inconsistent: cfg(crt-static) is enabled (respects the config file), but CARGO_CFG.. is not set.

@ohadravid

This comment has been minimized.

Copy link
Author

ohadravid commented Mar 20, 2019

@Eh2406 @alexcrichton something wasn't quite right, since the bug I saw didn't actually allow for the default flags to be passed.
I created a test which reproduces this (but which is NOT fixed by my change).
This actually has nothing to do with empty env args.

If you run it, it will actually fail in the second assert.

fn main() {
    // The crt-static feature should be enabled.
    #[cfg(not(target_feature = "crt-static"))]
    assert!(false);
    
    assert!(std::env::var("CARGO_CFG_TARGET_FEATURE").unwrap().contains("crt-static"));
}

(Changing the [target(cfg...)] to [target] will not trigger the bug).

I'm not really sure what to do, since requested_target in build_context/mod.rs:env_args seems to be set, but this seems like a contradiction (either both feature and env var should be set, or both shouldn't).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 20, 2019

Sorry I don't know off the top of my head what's going on, so I'd have to dig in, but I don't have time right now to do so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.