Actually, cargo doesn't "mostly just work"#444
Merged
Conversation
The `DEVELOPING.mkdn` file currently states that `cargo build` and `cargo test` "mostly just works". Actually, when building Humility for the first time, they may not just work, if the `cargo readme` cargo subcommand or `libudev` headers are missing. I've updated the `DEVELOPING.mkdn` file to state that these dependencies are necessary to build Humility.
This commit changes the `cargo xtask readme` xtask and the `humility-cmd-doc` build script to depend on `cargo-readme` as a library crate and call into its `generate_readme` function, rather than using it as an external binary that's invoked using `std::process::Command`. This way, the `cargo-readme` binary no longer needs to be installed using `cargo install` in order to build Humility instead, it's just a normal Cargo package dependency. In addition to making the first-time build experience a bit smoother by reducing the number of external dependencies that need to be installed, this also means our `cargo-readme` dependency is now a normal versioned crate dependency. That ensures that everyone developing Humility gets the `cargo-readme` version present in the lockfile, ensuring that `cargo-readme` generates consistent output. Installing the binary using `cargo-install` doesn't provide that guarantee, as it's not versioned, so if upstream changed their output format, the generated docs might change unnecessarily between different development systems with different `cargo-readme` versions installed.
Contributor
|
Oh, fantastic, the implicit reliance on |
cbiffle
reviewed
Feb 1, 2024
Contributor
cbiffle
left a comment
cbiffle
left a comment
There was a problem hiding this comment.
I like it, fwiw... should get @bcantrill 's eyes on it
bcantrill
approved these changes
Feb 3, 2024
Contributor
bcantrill
left a comment
bcantrill
left a comment
There was a problem hiding this comment.
This looks great -- thank you!
hawkw
added a commit
that referenced
this pull request
Feb 27, 2024
Currently, the Humility CI workflow attempts to run `cargo install cargo-readme`. Unfortunately, this is now broken on our MSRV, because `cargo-readme` won't build with un-locked dependencies on our MSRV (e.g. [this failed CI job][1]). Fortunately, however, we no longer actually need to install `cargo-readme` using `cargo-install`; as of #444, our build scripts now depend on it as a normal lib dep, instead. And because we depend on it as a normal Cargo lib dep, it's in our lockfile, and the version in the lockfile should (hopefully) build on MSRV. This commit removes the `cargo install cargo-readme` step from CI. Hopefully, this fixes the CI build... [1]: https://github.com/oxidecomputer/humility/actions/runs/8072478131/job/22054313203?pr=449
hawkw
added a commit
that referenced
this pull request
Feb 28, 2024
Currently, the Humility CI workflow attempts to run `cargo install cargo-readme`. Unfortunately, this is now broken on our MSRV, because `cargo-readme` won't build with un-locked dependencies on our MSRV (e.g. [this failed CI job][1]). Fortunately, however, we no longer actually need to install `cargo-readme` using `cargo-install`; as of #444, our build scripts now depend on it as a normal lib dep, instead. And because we depend on it as a normal Cargo lib dep, it's in our lockfile, and the version in the lockfile should (hopefully) build on MSRV. This commit removes the `cargo install cargo-readme` step from CI. Hopefully, this fixes the CI build... [1]: https://github.com/oxidecomputer/humility/actions/runs/8072478131/job/22054313203?pr=449
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
DEVELOPING.mkdnfile currently states thatcargo buildandcargo test"mostly just works". Actually, when building Humility forthe first time, they may not just work, if the
cargo readmecargosubcommand or
libudevheaders are missing.I've updated the
DEVELOPING.mkdnfile to state thatpkg-configandlibudevare necessary to build Humility.Additionally, I've changed the
cargo xtask readmextask and thehumility-cmd-docbuild script to depend oncargo-readmeas a librarycrate and call into its
generate_readmefunction, rather than using itas an external binary that's invoked using
std::process::Command. Thisway, the
cargo-readmebinary no longer needs to be installed usingcargo installin order to build Humility instead, it's just a normalCargo package dependency.
In addition to making the first-time build experience a bit smoother by
reducing the number of external dependencies that need to be installed,
this also means our
cargo-readmedependency is now a normal versionedcrate dependency. That ensures that everyone developing Humility gets
the
cargo-readmeversion present in the lockfile, ensuring thatcargo-readmegenerates consistent output. Installing the binary usingcargo-installdoesn't provide that guarantee, as it's not versioned,so if upstream changed their output format, the generated docs might
change unnecessarily between different development systems with
different
cargo-readmeversions installed.