-
-
Notifications
You must be signed in to change notification settings - Fork 70
Support Cargo build-dir #452
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
Conversation
1d28ef5 to
a2690a4
Compare
|
Ah, the scope of --target-dir's effect depends on whether build-dir is specified in the configuration, so this is blocked on taiki-e/cargo-config2#29. However, when using environment variables, it will probably not be a blocker. |
src/main.rs
Outdated
| // This is not the ideal way, but the way unstable book says it is cannot support them. | ||
| // https://doc.rust-lang.org/nightly/rustc/instrument-coverage.html#tips-for-listing-the-binaries-automatically | ||
| let mut target_dir = cx.ws.target_dir.clone(); | ||
| let mut build_dir = cx.ws.metadata.build_directory().to_owned(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely inefficient as it triggers a rebuild, but unfortunately, build-dir lacks a flag like --target-dir and only has transparent environment variables, so it may be difficult to make it behave the same as target-dir here.
| if let Some(target) = &cx.args.target { | ||
| target_dir.push(target); | ||
| build_dir.push(target); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to point out that the build-dir contents are considered internal to Cargo.
We are planning to restructure the build-dir layout in rust-lang/cargo#15947 which may break this logic in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though given the nature of this tool we probably have no choice but to depend on the Cargo internals to get the correct files :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though given the nature of this tool we probably have no choice but to depend on the Cargo internals to get the correct files :/
Yeah. This isn't only happening with Cargo; it can occur with other testing tools as well. (e.g., #299 (comment))
If changes occur, we'll address them. As for future changes, we can only hope that "upstream developers are smart enough and only change internal structures when it's actually reasonable to do so".
6356a68 to
f984df7
Compare
c4efa6e to
6aa7b3d
Compare
Fixes #432
There is no support for nextest-archive because nextest itself doesn't seem to support it yet.