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

Allow cargo:rustc-env in build scripts #3929

Merged
merged 4 commits into from May 15, 2017
Merged

Allow cargo:rustc-env in build scripts #3929

merged 4 commits into from May 15, 2017

Conversation

Xion
Copy link
Contributor

@Xion Xion commented Apr 17, 2017

This is an attempt to address issue #2875. Basically, I'm trying to allow build scripts to producecargo:rustc-env=FOO=foo lines in their output. These are then translated to env!()-friendly env. vars that rustc is run with.

@rust-highfive
Copy link

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.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good to me, thanks @Xion!

I wonder if we want to maybe pursue namespacing the env vars though? Righw now cargo exports env vars as CARGO_* and we may wish to scope the custom env vars here to ensure they don't clash with future significant env vars and such.

build_state: &BuildMap,
build_scripts: &BuildScripts,
current_id: &PackageId) -> CargoResult<()> {
for key in build_scripts.to_link.iter() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be a loop, it can just be using build_state.get(current_id), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current_id is just a PackageId, and to key into BuildMap I also a cargo_rustc::Kind which I'm not entirely clear about the purpose of. I picked Kind::Host following the other similar usages and it seems to work no worse than the previous version.

"#)
.file("src/main.rs", r#"
const FOO: &'static str = env!("FOO");
fn main() {}
Copy link
Member

Choose a reason for hiding this comment

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

Could this assert the right value of FOO as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@alexcrichton
Copy link
Member

Note that another use case in #2875 is to export git information which I think Cargo should just do by default.

In any case I feel like this is a totally fine feature to have, it's certainly more ergonomic than using include!!

@alexcrichton alexcrichton added the relnotes Release-note worthy label Apr 20, 2017
@alexcrichton
Copy link
Member

cc @matklad, @carols10cents, @wycats, @brson, an upcoming feature in Cargo!

@petrochenkov
Copy link
Contributor

@Xion
Build script overrides need to be updated as well.

@matklad
Copy link
Member

matklad commented Apr 21, 2017

Looks great! It needs to be documented though :)

@bors
Copy link
Contributor

bors commented May 8, 2017

The latest upstream changes (presumably #3974) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

ping @Xion, would you be willing to rebase and/or add docs?

@Xion
Copy link
Contributor Author

Xion commented May 12, 2017

Sorry, I've been busy with onboarding at a new job. I'll try to get to it this weekend.

@alexcrichton
Copy link
Member

No worries! Let me know if you run out of time for this and I'd be happy to take it over

"#)
.file("src/main.rs", r#"
const FOO: &'static str = env!("FOO");
fn main() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

build_state: &BuildMap,
build_scripts: &BuildScripts,
current_id: &PackageId) -> CargoResult<()> {
for key in build_scripts.to_link.iter() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

current_id is just a PackageId, and to key into BuildMap I also a cargo_rustc::Kind which I'm not entirely clear about the purpose of. I picked Kind::Host following the other similar usages and it seems to work no worse than the previous version.

}

#[test]
fn env_test() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that this test still fails at the build step (with FOO variable not being defined). There is apparently something more that needs to be done to carry the variables if the build scripts is invoked for a crate as dependency for the test crate.

@Xion
Copy link
Contributor Author

Xion commented May 12, 2017

As for namespacing, what prefix would not clash with any of the tooling? I was thinking about RUSTC_ENV_ to match the script output, or CARGO_ENV_ to call dibs on that one already.

On the other hand, I'm not sure such prefixing is necessary nor desirable. Since the values for the env vars are purely in the user's discretion, the names should be too. There may even be some valid use cases for shadowing the CARGO_ variables -- which the user can already do by invoking CARGO_FOO=x cargo rustc -- so I wouldn't preclude that either.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Yeah I think you're right in that we may wish to eschew the prefix, that sounds ok to me.

I think there's some tests failing here related to rustdoc as well? Could you also add tests that execute cargo doc to ensure that works as well?

build_state: &BuildMap,
_: &BuildScripts,
current_id: &PackageId) -> CargoResult<()> {
let key = (current_id.clone(), Kind::Host);
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes here you'll want to pass in a kind instead of just picking Kind::Host, which I believe will break cross-compiled builds. Typically most rules have unit.kind which can be ferried to this locaion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so that's what host vs target means. Makes sense! Fixed.

// been put there by one of the `build_scripts`) to the command provided.
fn add_custom_env(rustc: &mut ProcessBuilder,
build_state: &BuildMap,
_: &BuildScripts,
Copy link
Member

Choose a reason for hiding this comment

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

Ah feel free to just remvoe this parameter if it's not used.

@Xion
Copy link
Contributor Author

Xion commented May 13, 2017

I've fixed the problem the test for cargo test, and also added one for rustdoc. Documentation is also included.

@alexcrichton
Copy link
Member

@bors: r+

Looks great to me, thanks so much for tackling this @Xion!

@bors
Copy link
Contributor

bors commented May 15, 2017

📌 Commit 8e6ffa5 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 15, 2017

Testing commit 8e6ffa5 with merge 6709508...

@bors
Copy link
Contributor

bors commented May 15, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented May 15, 2017 via email

@bors
Copy link
Contributor

bors commented May 15, 2017

Testing commit 8e6ffa5 with merge 00af72e...

bors added a commit that referenced this pull request May 15, 2017
Allow cargo:rustc-env in build scripts

This is an attempt to address issue #2875. Basically, I'm trying to allow build scripts to produce`cargo:rustc-env=FOO=foo` lines in their output. These are then translated to `env!()`-friendly env. vars that rustc is run with.
@bors
Copy link
Contributor

bors commented May 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 00af72e to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants