search

obi1kenobi / cargo-semver-checks-action

A GitHub Action for running cargo-semver-checks
MIT License
60 stars 15 forks source link

Add target input #82

Closed memark closed 3 months ago

memark commented 4 months ago

This PR adds an input rust-target, that maps directly to the --target argument of the underlying binary.

Solves https://github.com/obi1kenobi/cargo-semver-checks-action/issues/54.

About the name, just target would make more sense, but the toolchain option is already named rust-toolchain, so I decided to stick with that prefix. Not sure which is better.

I could use some help in how to test this. Some simple test to see that the option is passed-through, or some advanced test where the testet crate actually has a different API surface depending on the selected target? Please advice.

obi1kenobi commented 4 months ago

I could use some help in how to test this. Some simple test to see that the option is passed-through, or some advanced test where the testet crate actually has a different API surface depending on the selected target? Please advice.

I'm not sure, to be honest. I've never used this option so I don't know how to test it.

Perhaps one of the projects that might use this functionality can be used in the form of a test case? We do something similar for regression-testing on cargo-semver-checks: we check out another project's repo at a specific commit, then run the tool and assert that the expected outcome is reached. For example: https://github.com/obi1kenobi/cargo-semver-checks/blob/5108eeeffaf424e8d1a0019534414e7f11276a66/.github/workflows/ci.yml#L1207

The key considerations are that the project should be reasonably well-known and well-supported (including on newer Rust versions), not too large (so the clone doesn't take a long time), and that semver-checking it shouldn't take too long either.

memark commented 3 months ago

This looks good and I'd be happy to merge it. Thank you!

You're most welcome. Glad I could help.

I have a small suggestion for an extra line in the docs that I'd love if you considered.

Sure thing!

Other than that, I think this just needs a rebuild of the JS files and it's good to go.

Done.

(Also, as a general rule, I won't merge a PR that the author still considers a draft since I feel that would be going against the author's wishes, so I'll wait for you to mark it ready as well.)

That's a good principle. I have some more testing I want to do before I'll move it out of draft.

Note that the nightly Rust CI builds will fail right now, and that's not anything you did. The rustdoc JSON format is very much in flux on nightly at the moment with some edition-2024-related changes. I'm giving it a few days for all the breaking changes currently in the pipeline to get merged and published before I release a new cargo-semver-checks version that supports the final result.

Noted.

memark commented 3 months ago

Alright, I did some more testing in a separate branch of my own, just to verify that things work as expected. These are the most interesting steps

Specifying a target that's not installed, the action not installing it https://github.com/memark/cargo-semver-checks-action/actions/runs/9929514867/job/27427194344#step:6:21

Specifying a target that's not installed, letting the action install it https://github.com/memark/cargo-semver-checks-action/actions/runs/9929514867/job/27427194344#step:7:41

With these tests I'm happy with the results and the PR.

obi1kenobi commented 3 months ago

Looks great! I opened #83 to tweak the CI configuration so we can run the full test suite even though nightly is currently failing, and will merge this right afterward.

Thanks again for the excellent contribution — I really appreciate the craft and attention to detail you clearly put into this work!

memark commented 3 months ago

Thank you so much for your kind words, I really appreciate them!

obi1kenobi commented 3 months ago

Released as v2.5 of the action, and moved the v2 tag to point to v2.5 as well.