ci: cargo-semver-checks#2946
ci: cargo-semver-checks#2946apoelstra merged 2 commits intorust-bitcoin:masterfrom storopoli:ci/cargo-semver-checks
cargo-semver-checks#2946Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
@obi1kenobi, I think that for the two items in the TODO list above we cannot do it with the I'd probably need to run |
|
To not fail the workflow, you could use GitHub Actions' The labeling workflow is not possible with the current action, since it always uses the version on crates.io as baseline, not the repo's main branch. That means that after you merge the first breaking change, all subsequent workflow runs until the next release will re-discover that breaking change since they will be comparing "PR to crates.io" not "PR to target branch." I want this to get solved in the action, but that's blocked on finding more funding right now. If you come up with a workflow you like, I'd love to see it and possibly even accept a PR to the action. One more caveat: support for nightly Rust versions in |
|
Sounds good. We'll dig into what's needed to use a different baseline. Presumably we won't be able to use the action, and instead will need to hack up some shell scripts to do it for us. This shouldn't be too bad. We have some experience with our current label-bot doing things like this. |
|
Thank you, I really appreciate it!
I've also wanted to make the action annotate the code: post notes or comments on specific lines where breakage happened. I'll describe the stumbling blocks I ran into, in case that's helpful:
|
What you need to do is somehow export the information to other actions and have a flag to not fail. Then other labeling action can take that and use it. Though to be strictly secure it'd need to be another job I think. (Not a big problem I guess.) |
|
Apparently And we need different stuff (see https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/blob/3494ceec521cbc2cbe7e4b0d3fb9219a2164f57d/ci/run_task.sh#L277) So, I will try to generate the JSON files manually and call EDIT: In the shell script I can generate the JSON doc files with |
|
@storopoli we only need those |
|
This is probably obvious and you were probably already planning to do what I recommend here, but it's important so mentioning it just in case: I would recommend against reusing the rustdoc built in the existing I would strongly recommend reusing the exact For example:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Pull Request Test Coverage Report for Build 9766164784Details
💛 - Coveralls |
It really shouldn't. We only use it to enable the unstable feature that says which items are activated by which features.
OMG 🤦♂️ 🤦♂️ 🤦♂️ |
Pull Request Test Coverage Report for Build 9766958169Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9767018028Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9774220698Details
💛 - Coveralls |
This comment was marked as resolved.
This comment was marked as resolved.
Pull Request Test Coverage Report for Build 9774697799Details
💛 - Coveralls |
contrib/check-semver.sh
Outdated
There was a problem hiding this comment.
You should generate these programmatically.
There was a problem hiding this comment.
I don't follow. Programmatically means "in a way that follows a plan or uses a particular method".
That can be anything. Can you be more specific?
There was a problem hiding this comment.
I believe he means "extract the names of the crates from the top-level Cargo.toml file".
I wouldn't worry about this warning. I wish we could silence it. It's true that the |
Pull Request Test Coverage Report for Build 9779513941Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9779635068Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9779726987Details
💛 - Coveralls |
I think the warning is correct. The metadata in the requirement field is noop, so putting it there is equivalent to writing a comment which doesn't look like a comment, so it's confusing. We can achieve the same by writing a comment and then whoever reads it know it's just a comment and they can learn the true version by running |
Pull Request Test Coverage Report for Build 9779939590Details
💛 - Coveralls |
.github/workflows/rust.yml
Outdated
There was a problem hiding this comment.
Depending on the threat model, we can speed this up with:
| - name: "Install cargo-semver-checks" | |
| run: cargo install cargo-semver-checks --locked | |
| - name: "Install cargo-binstall" | |
| uses: cargo-bins/cargo-binstall@main | |
| - name: "Binstall cargo-semver-checks" | |
| run: cargo binstall cargo-semver-checks --no-confirm |
There was a problem hiding this comment.
I think it's fine. We're not checking the signatures/hashes of the source either.
Pull Request Test Coverage Report for Build 9784547300Details
💛 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 9784547300Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9784547300Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9784568584Details
💛 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 9784568584Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9784568584Details
💛 - Coveralls |
tcharding
left a comment
There was a problem hiding this comment.
Awesome work man, I can see you put in a whole lot of effort! I have a few minor nits but I think we should merge as is and improve in follow ups if we want to, bash always has a few nits :)
To be explicit, and for other reviewers these are the issues I see that I think are ok to merge:
- code duplication
- code comments
- I think
bitcoin_hashesmight end up beingbitcoin-hashesin some file names
Thanks again @storopoli, props on this one.
|
This PR should probably also remove the |
|
Yeah, let's merge this ASAP. I'll review it later since removing api dir is a good idea and I'm exhausted now. |
Removes check-api workflow, helper scripts, documentation, and files.
Yes, good catch. Did this in the proper commit d9567b0. |
Pull Request Test Coverage Report for Build 9792465705Details
💛 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 9792465705Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9792465705Details
💛 - Coveralls |
apoelstra
left a comment
There was a problem hiding this comment.
ACK bc2b980b10c94e2872ca1480eb11c7e0cb16b22e Looks great -- definitely want to circle back to improve the comment it posts, try to clean up some repeated code, and possibly add a larger feature matrix
| # and then using those files to check for breaking changes with | ||
| # cargo semver-checks. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
I always read this as "euro" :D
| # Run cargo doc with the cargo semver-checks rustdoc flags. | ||
| # We don't care about dependencies. | ||
| run_cargo_doc() { | ||
| RUSTDOCFLAGS="$RUSTDOCFLAGS" cargo +"$NIGHTLY" doc --no-deps "$@" |
There was a problem hiding this comment.
Oh, wait, does it catch public deps anyway or not?
| # Hack to not fail on errors. | ||
| # This is necessary since cargo semver-checks will fail if the | ||
| # semver check fails. | ||
| # We check that manually later. |
There was a problem hiding this comment.
There's a less brittle hack for that: append || true at the end of the command.
There was a problem hiding this comment.
Yeah that was my initial implementation.
| if ! [ -f semver-break ]; then | ||
| echo "No breaking changes found" | ||
| fi | ||
| } |
There was a problem hiding this comment.
Now I really don't understand why it's done here and not after the check.
There was a problem hiding this comment.
This is if all cargo semver-checks got a FAIL.
There was a problem hiding this comment.
Oh, doing it in a loop would create the comment multiple times, right?
Damn, I missed this during review. This claim is not correct. Its missing "all features" for # Uses `CARGO` to generate API files in the specified crate.
#
# Files:
#
# - no-features.txt
# - alloc-only.txt
# - all-features.txt
generate_api_files() {
local crate=$1
pushd "$REPO_DIR/$crate" > /dev/null
run_cargo --no-default-features | $SORT | uniq > "$API_DIR/$crate/no-features.txt"
run_cargo --no-default-features --features=alloc | $SORT | uniq > "$API_DIR/$crate/alloc-only.txt"
run_cargo_all_features | $SORT | uniq > "$API_DIR/$crate/all-features.txt"
popd > /dev/null
}Was there a reason for this @storopoli? |
|
@tcharding no, my intention was to mimic what we did with
But yeah, I need to add the |
|
My list is not correct, the |
check-api(cargo-public-api)cargo-semver-checksNote to Reviewers
There is a new script
contrib/check-semver.shthat checks for semver breaks againstmaster.If it detects a breaking change it will create the
.semver-breakfile.If this file exists then we will add an
API breaklabel and a big comment to the PR saying:It reproduces the current behavior of
cargo-public-api:bitcoin:all-featuresno-default-featuresbase58ck:all-featuresno-default-featuresbitcoin_hashes:no-default-featuresfeatures allocbitcoin-units:no-default-featuresfeatures allocbitcoin-io:no-default-featuresfeatures allocCloses #1875.
Supersedes #2912.
Context
Our current test to check API breaks using
cargo public-apiis not "automated" per se.It needs contributors to keep tabs on text files with function signatures and other things and forces reviewers to be the de facto API break detector tool.
The idea is to use the well-acclaimed and maintained
cargo semver-checksthat will do this automatically in CI.