-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Implement host
-target substitution
#15838
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
base: master
Are you sure you want to change the base?
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
In writing up this PR's explanation, I was unsure how to properly outline the following:
... due to my unfamiliarity with how testing the program that tests programs would exactly work. Perhaps, simply clone the PR and try to run Thanks. |
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.
Thanks for the contribution. We'll need some tests for it, as well as documentation update.
- For documenting, this is the place to go: https://github.com/rust-lang/cargo/blob/master/src/doc/man/includes/options-target-triple.md. Also see how to regenerate docs here.
- For writing tests, see https://doc.crates.io/contrib/tests/index.html. I would assume it would be some functional tests, at least one for
--target host
, the other forbuild.target = ["host"]
. Those tests may go intotests/testsuite/build.rs
file. (multitarget.rs might be another candidate sincebuild.rs
has become too big a file).
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.
Cases we care about
--target host
andbuild.target = ["host"]
work and are treated as cross-compilation- de-duplication of targets works
- that
"host"
in other contexts doesn't accidentally work
As a tip on test writing: you can add the tests in a commit before, doing a "{SOMETHING}"..replace(cargo_test_support:rustc_host())
. Then in this commit, you can replace that with host"
and nothing else in the test should change, making it clear through the diff that "host"
truly has the same behavior as --target <something>
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.
Thanks for the contribution. We'll need some tests for it, as well as documentation update.
- For documenting, this is the place to go: https://github.com/rust-lang/cargo/blob/master/src/doc/man/includes/options-target-triple.md.
Do you feel this is an acceptable addition to the documentation?
...
{{~/if}} The general format of the triple is
`<arch><sub>-<vendor>-<sys>-<abi>`. Run `rustc --print target-list` for a
list of supported targets.
my addition
v v v v v v v
In addition to the supported targets, you may specify `host` as the target. This has the same behavior as specifying a regular target, and produces a build for the host machine's architecture.
{{~#if multitarget }} This flag may be specified multiple times. {{~/if}}
...
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.
--target host and build.target = ["host"] work and are treated as cross-compilation
So probably put tests under tests/testsuite/cross_compile.rs
. Sounds good.
Do you feel this is an acceptable addition to the documentation?
Looks great, though is it possible to be a bit more concise?
--jobs
flag has this for the "default" string:
-j N, --jobs N
Number of parallel jobs to run. May also be specified with the
...
If a string default is provided, it sets the value back to defaults.
Should not be 0.
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.
@weihanglo I opted to rewrite the documentation, as I felt the original was a bit incomplete (didn't mention custom target specifications, for instance). I understand if the change is too dramatic, please let me know.
{{#option "`--target` _triple_"}}
{{actionverb}} for the specified target {{~#if multitarget }} (may be specified multiple times) {{~/if}}
{{~#if target-default-to-all-arch}} The default is all architectures.
{{~else}} The default is the host architecture.
{{~/if}} The general format of the triple is
`<arch><sub>-<vendor>-<sys>-<abi>`.
You may specify the following kinds of targets:
- Any supported target in `rustc --print target-list` (note: you have to add the target to use it).
- `host`, which will internally be substituted by the host's target. This can be particularly useful if you're cross-compiling some crates, and don't want to specify your host's machine as a target (for instance, an `xtask` in a shared project that may be worked on by many hosts).
- A path to a custom target specification (further reading [here](https://doc.rust-lang.org/rustc/targets/custom.html#custom-target-lookup-path)).
This may also be specified with the `build.target` [config value](../reference/config.html).
**Note**: Specifying this flag makes Cargo run in a different mode where the target artifacts are placed in a separate directory. See the [build cache](../reference/build-cache.html) documentation for more details.
{{/option}}
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.
@epage Would adding specific tests for the host
target specifier be acceptable? It seems like bit of a special case, whose implementation is somewhat easy to miss (being just a few lines inside a rather deeply nested call stack), so I think a couple tests to ensure the specifier works as expected into the future would be a good idea.
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.
I feel I'm missing something in the question because it sounds like what I said was needed (--target host
tests)
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.
@epage This bit seemed to suggest to me that you wanted an impermanent test (showing soundness via the diff, rather than a persistent test):
As a tip on test writing: you can add the tests in a commit before, doing a "{SOMETHING}"..replace(cargo_test_support:rustc_host()). Then in this commit, you can replace that with host" and nothing else in the test should change, making it clear through the diff that "host" truly has the same behavior as --target
... though I admittedly didn't really understand what you wrote. I'll go ahead and push the tests!
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.
Not an impermanent test but a suggestion on how to structure commits in the PR that help demonstrate what the PR does and help test the tests. See also https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request
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.
Doc looks fine. You might also want to update its config doc: https://doc.rust-lang.org/nightly/cargo/reference/config.html#buildtarget.
src/cargo/util/command_prelude.rs
Outdated
} | ||
} | ||
|
||
// Allow tab-completion for `host` as the desired target. | ||
candidates.push(clap_complete::CompletionCandidate::new("host")); |
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.
Can you read env!("RUST_HOST_TARGET")
and put that in the help string in some way, like alias: {}
?
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.
Definitely, but where exactly do these completions show up? And their .help()
strings. I assumed it was tab-completion, but navigating to target/debug/
and tab-completing ./cargo build --target
only provides one option (the only installed target, which is the host architecture).
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.
Do you have our unstable completions configured and using your custom of cargo?
Whether the help string shows up is dependent on the shell. Clap supports it through some shells native mechanisms. There are ways to hack it in to some shells but haven't done that yet.
I have additionally tested locally that the following
Should I somehow mirror this within the PR, rather than just ensuring locally that builds with the above conditions do not compile? I think I could write an always-fail test that does this, but I wanted to make sure that's the means we'd like the above no-compile conditions to be met. |
What does this PR try to resolve?
This PR resolves: #13051
Namely, it allows users to invoke cargo subcommands that accept a
--target
directive to specify thehost
target, which is later substituted in the command processing layer by the host's real target triple (for instance, on most Linux distributions,cargo build --target host
would effectively runcargo build --target x86_64-unknown-linux-gnu
).This additionally applies to usage within
config.toml
, like so: