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

`OUT_DIR` variable not set #3368

Closed
Razican opened this issue Dec 4, 2016 · 27 comments

Comments

@Razican
Copy link

commented Dec 4, 2016

Some Travis jobs are not compiling due to OUT_DIR not being present on nightly on my build.rs script. Is this an expected behaviour? how could it be fixed?

Example failure: https://travis-ci.org/SUPERAndroidAnalyzer/super/jobs/181113222#L146

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

I can reproduce this with "cargo install ripgrep" using a cargo built from latest cargo git. Debugging...

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

Looks like commit 41579ba caused this issue.

@upsuper

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2016

This breaks building servo/rust-bindgen as well.

@JIghtuse

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2016

cargo even has failing test for me with the same symptoms: http://paste.fedoraproject.org/500936/48110771/

Can't build ripgrep too.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2016

I’ve managed to build bindgen by replacing env! in the build script with std::env::var: rust-lang/rust-bindgen#323

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2016

Still, it looks like this is a breaking change. So Cargo should probably revert to providing these variables when compiling a build script. At least for a while, with a warning that this will change.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

It looks like commit 95193ca clarified the documentation to state that Cargo doesn't set these environment variables when building the build script, only when running it, even though Cargo did actually set them when building the build script as well. A later commit in Cargo then made the behavior match the documentation.

This does indeed represent a breaking change from the existing behavior, and existing build.rs scripts relied on the existing behavior.

knight42 added a commit to uutils/coreutils that referenced this issue Dec 8, 2016
Temporary fix for testing errors
The errors were caused by the missing env $OUT_DIR which should be set by
cargo, see [here](rust-lang/cargo#3368).
knight42 added a commit to uutils/coreutils that referenced this issue Dec 8, 2016
Temporary fix for errors in testing
The errors were caused by the missing env $OUT_DIR which should be set by
cargo.

[Related issue](rust-lang/cargo#3368).
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

Ah this is definitely an accidental regression! Yes env!("OUT_DIR") is likely not what you want but rather std::env::var is correct in build scripts. The env! version is the out dir that the build script was built with, and the env::var version is for when the build script is being run.

I'm kinda surprised that env! ever worked...

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

Well... so this was a "regression" but also a bug fix at the same time. The OUT_DIR environment variable can be different when the build script is being compiled compared to when the build script is being run. Notably during cross compilation these two can be different. This means that crates which use env!("OUT_DIR") I believe are broken for cross compiles where crates which use env::var("OUT_DIR") will correctly work with cross compiles.

To those hitting this issue, could you confirm that the crate hasn't been used for cross compiles? And perhaps if it has have weird issues arisen?

We could add a fix for this but I'm tempted to avoid doing so if we can (as it's "incorrect" to rely on the old behavior). If it's possible to fixup the packages in question quickly that'd be best, but if there's ongoing pain then I can change Cargo back to the previous behavior.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

@alexcrichton I've seen various crates get fixed for this change (which, at a minimum, needs documentation in release notes).

It'd be good to be able to build existing packages with new Cargo. For the most part, Cargo has maintained backward compatibility across many versions. On the other hand, I do agree that crates shouldn't rely on the old behavior, and it would indeed have broken cross-compiles.

I don't see an obvious way to work-but-warn in this case, without awful hacks (such as somehow substituting in a version of env! that checks the environment variable name and produces a warning).

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 11, 2016

Yes my only worry is if crates can't be updated. Crates can use std::env::var and work across all versions of Cargo, so if a crate can be proactively updated I'm not too worried about it.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2016

Can crater be used to find out what on crates.io is broken by this change?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 13, 2016

@SimonSapin I believe so, yes, we should see it in the next crater run (as soon as it's working)

brendanzab added a commit to brendanzab/gluon that referenced this issue Dec 18, 2016
alexcrichton added a commit to alexcrichton/rust-pinyin that referenced this issue Dec 28, 2016
Use env::var_os intead of env! in build script
The env! method pulls the variable at *compile time* as opposed to runtime. This
can cause breakage in scenarios with cross compilation, for example. Currently
there's also a pending change to Cargo on the beta release of Rust which breaks
the usage of env! (rust-lang/cargo#3368).

We may roll that change back, but I figured it'd be good to head off future
breakage anyway!
alexcrichton added a commit to alexcrichton/rust-pinyin that referenced this issue Dec 28, 2016
Use env::var_os intead of env! in build script
The env! method pulls the variable at *compile time* as opposed to runtime. This
can cause breakage in scenarios with cross compilation, for example. Currently
there's also a pending change to Cargo on the beta release of Rust which breaks
the usage of env! (rust-lang/cargo#3368).

We may roll that change back, but I figured it'd be good to head off future
breakage anyway!
alexcrichton added a commit to alexcrichton/zip_codes that referenced this issue Dec 28, 2016
Use env::var_os intead of env! in build script
The env! method pulls the variable at *compile time* as opposed to runtime. This
can cause breakage in scenarios with cross compilation, for example. Currently
there's also a pending change to Cargo on the beta release of Rust which breaks
the usage of env! (rust-lang/cargo#3368).

We may roll that change back, but I figured it'd be good to head off future
breakage anyway!
@brson

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2016

gazetta-bin-0.1.6 hits this.

alexcrichton added a commit to alexcrichton/gazetta-bin that referenced this issue Dec 29, 2016
Use env::var intead of env! in build script
The env! method pulls the variable at *compile time* as opposed to runtime. This
can cause breakage in scenarios with cross compilation, for example. Currently
there's also a pending change to Cargo on the beta release of Rust which breaks
the usage of env! (rust-lang/cargo#3368).

We may roll that change back, but I figured it'd be good to head off future
breakage anyway!
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 29, 2016

@brson

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2016

pinyin-0.0.5 hits this. (Edit: oh you've fixed pinyin)

@brson

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2016

zip_codes-0.0.1 is affected cc @SkylerLipthay

@SkylerLipthay

This comment has been minimized.

Copy link

commented Dec 31, 2016

Alex fixed this with SkylerLipthay/zip_codes#2, which has been merged! zip_codes 0.1.0 was subsequently released.

@brson

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2016

@brson

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2016

Great @SkylerLipthay! Sorry for the inconvenience.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

@brson I think pinyin and rasen were both addressed here -- rust-lang/rust#38391 (comment)

@brson

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2017

lmdb-rs 0.7.3's test suite is affected. cc @vhbit

vhbit added a commit to vhbit/lmdb-rs that referenced this issue Jan 5, 2017
Fixes broken tests on nightly
Caused by rust-lang/cargo#3368.
Now the method for determining path for DB tests is exact copy
from `cargo`.
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

@brson looks like that was fixed by vhbit/lmdb-rs@25c1fdc

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

Ok we've decided to close this as working as intended, but if any projects need help migrating please just let me know!

@abonander

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alexcrichton The story isn't over, however: abonander/mime_guess#33

This invocation is at the build time of the crate: https://github.com/abonander/mime_guess/blob/master/src/lib.rs#L16

When OUT_DIR should be defined at that point because there is a build script.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@abonander maybe the version of Cargo there is too old to recognize the implicit build script via build.rs?

@abonander

This comment has been minimized.

Copy link

commented Mar 12, 2018

@alexcrichton Good call! The issue reporter was running a Docker container with a super-outdated Cargo.

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