-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
tests: refactor testsuite cargo build --target
#14843
tests: refactor testsuite cargo build --target
#14843
Conversation
Add three additional functions to Execs in order to more cleanly pass a target triple, host target or target directory when running a test case.
@@ -140,7 +145,10 @@ fn custom_bin_target() { | |||
.file("custom-bin-target.json", SIMPLE_SPEC) | |||
.build(); | |||
|
|||
p.cargo("build --target custom-bin-target.json -v").run(); | |||
p.cargo("build") |
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.
One drawback of it is that the argument list is spliited into multiple lines of indirection, making it harder to read at first glance.
@@ -53,8 +53,13 @@ fn custom_target_minimal() { | |||
.file("custom-target.json", SIMPLE_SPEC) | |||
.build(); | |||
|
|||
p.cargo("build --lib --target custom-target.json -v").run(); | |||
p.cargo("build --lib --target src/../custom-target.json -v") | |||
p.cargo("build --lib") |
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.
So does this one. The original one is more readable IMO.
@@ -494,7 +494,7 @@ fn custom_build_env_var_rustc_linker_with_target_cfg() { | |||
|
|||
// no crate type set => linker never called => build succeeds if and | |||
// only if build.rs succeeds, despite linker binary not existing. | |||
p.cargo("build --target").arg(&target).run(); | |||
p.cargo("build").target(&target).run(); |
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 kind of dynamic one is something in my mind could benefit from this change, though I haven't seen other 100+ occurrences changed in this pull request.
Upon this point, maybe this refactor might not be worthy with this scale of churn on your side?
(updating 100+ occurrences of --target
sounds tedious)
Just chatted with the PR author on Zulip. This is likely a bit too tedious to change 100+ occurrences all at once. They are not going to chase this down at this cmoment and instead focusing on the original issue #14183. Thanks for their contribution anyway :) |
What does this PR try to resolve?
Mentioned here, this PR is a small refactor to add three additional functions to
Execs
in order to pass a target triple, host target or target directory more cleanly when running a test case.