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

Add test case for OUT_DIR in tests. #954

Merged
merged 8 commits into from
Oct 16, 2021

Conversation

sayrer
Copy link
Contributor

@sayrer sayrer commented Sep 22, 2021

This doesn't pass right now, but it does pass under cargo test. I see the rustc rule has a way to pass OUT_DIR, but it looks like tests aren't getting this right now. Posting this now, before I dig further, in case someone knows of a quick fix here.

It looks like this cargo behavior is intentional: rust-lang/cargo#879

@google-cla google-cla bot added the cla: yes label Sep 22, 2021
@sayrer
Copy link
Contributor Author

sayrer commented Sep 23, 2021

I decided it's probably not worth it to emulate cargo here, after thinking about it some more.

This patch should probably land for test coverage and documentation purposes. I searched the repo for OUT_DIR while trying to figure out how to make it work in integration tests.

@hlopko
Copy link
Member

hlopko commented Sep 27, 2021

I'm a bit confused, is the test supposed to fail with bazel at the moment? Because the CI seems to be green. Do we have a bug in the CI configuration?

@sayrer
Copy link
Contributor Author

sayrer commented Sep 27, 2021

I'm a bit confused, is the test supposed to fail with bazel at the moment? Because the CI seems to be green. Do we have a bug in the CI configuration?

No--it all works, it's just undocumented (although implied by rust_test kwargs) and uncovered by tests, I think. I had to write this PR to figure it out. This PR tests passing OUT_DIR to integration tests at compile time, and shows that Bazel does not set OUT_DIR at test runtime. Cargo does set OUT_DIR at runtime, but that happened by mistake, and they are having trouble undoing this behavior now (the docs still insist OUT_DIR is only available at compile time).

Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

Overall it's a nice PR, thank you! To me it makes sense to check it in. Only one nit remaining.

test/out_dir_in_tests/Cargo.toml Outdated Show resolved Hide resolved
@UebelAndre UebelAndre merged commit cdca6ed into bazelbuild:main Oct 16, 2021
@sayrer sayrer deleted the out_dir_in_tests branch October 16, 2021 20:38
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.

None yet

4 participants