Detect when build.target is configured, and allow overriding it from the command line
#619
Conversation
|
Figured it out, I use |
Nemo157
force-pushed
the
detect-build-target
branch
3 times, most recently
from
d118094
to
56bfbbd
Compare
Might you be willing to open a PR that adds a "troubleshooting" section to our CONTRIBUTING.md doc and describes this problem and its solution? I'd imagine it would be useful to other people in the future as well. |
src/main.rs
Outdated
| /// Which target to build the crate for, to check platform-specific APIs. | ||
| #[arg(long)] | ||
| target: Option<String>, |
There was a problem hiding this comment.
Thoughts on calling this field build_target instead (+updating all other internal names to build_target), then making an alias for --target to match cargo?
Cargo seems split: it calls it --target, build.target, and CARGO_BUILD_TARGET. I'm inclined to support --target to match it in the CLI, but I feel like internally and via the lib API build_target is going to be a less confusing name. For example, we don't want it to be confused with a custom target directory, which one day might also be another Option<String> field with target in the name.
There was a problem hiding this comment.
It might also be nice to add an example of a build target in the doc comment, say e.g. x86_64-unknown-linux-gnu
src/lib.rs
Outdated
| @@ -299,6 +301,11 @@ impl Check { | |||
| self | |||
| } | |||
|
|
|||
| pub fn with_target(&mut self, target: String) -> &mut Self { | |||
There was a problem hiding this comment.
nit: this is a public function so let's document what it does, what's the default value if this function isn't used, etc. It's okay if it mirrors some of the CLI's documentation.
src/lib.rs
Outdated
| @@ -37,6 +37,7 @@ pub struct Check { | |||
| release_type: Option<ReleaseType>, | |||
| current_feature_config: rustdoc_gen::FeatureConfig, | |||
| baseline_feature_config: rustdoc_gen::FeatureConfig, | |||
| target: Option<String>, | |||
There was a problem hiding this comment.
Probably a good idea to add a field-level doc comment here, describing what the field means and what the value None means.
This allows checking the correct directory for the json files. There are
two more correct approaches to this, but neither are usable currently:
1. The most correct approach would be to read the `compiler-artifact`
messages from `cargo --message-format=json`. Unfortunately as of the
current nightly these don't work when using `--output-format=json`,
they have an empty list of files.
2. The next option would be to use `--out-dir` to tell cargo/rustdoc to
place the json file in a known directory. As of the current nightly
cargo doesn't support `--out-dir` with `cargo doc`, and attempting
to pass it through to rustdoc directly results in an error because
it is given more than once.
So instead, we query cargo to see if the user has configured
`build.target` through either a config file or environment variables.
Nemo157
force-pushed
the
detect-build-target
branch
from
dbe066c
to
dfa38b7
Compare
|
Nicely done, love how carefully you put all this together! 🚀 Thank you so much! If you might be interested in making more PRs and want some ideas, give me a ping anytime. I've written up some of my thinking as GitHub issues, but not everything is in there yet. |
This allows checking the correct directory for the json files. There are two more correct approaches to this, but neither are usable currently:
compiler-artifactmessages fromcargo --message-format=json. Unfortunately as of the current nightly these don't work when using--output-format=json, they have an empty list of files.--out-dirto tell cargo/rustdoc to place the json file in a known directory. As of the current nightly cargo doesn't support--out-dirwithcargo doc, and attempting to pass it through to rustdoc directly results in an error because it is given more than once.So instead, we query cargo to see if the user has configured
build.targetthrough either a config file or environment variables.fixes #586, #579
Also, because it was easy after fixing the above I added a CLI flag to allow setting which target to build for.
fixes #579 more 😁