V2 of the action

[mgr0dzicki]

Uses cargo-semver-checks built-in logic instead of the bash scripts (closes #23).
Introduces option manifest-path (closes #18).
Introduces option verbose (closes #16).
Uses pre-built binaries (closes #8).

Because the default behavior is changed and inputs crate-target and version-tag-prefix are removed, a version bump to v2 is necessary.

[mgr0dzicki]

While testing the draft of new action on GitHub windows-latest runner, I ran into a following error:

C:\Users\runneradmin\.cargo\bin\cargo.exe semver-checks check-release
Error: failed to make directory 'C:\Users\runneradmin\.cargo\registry/index\github.com-1ecc6299db9ec823': The system cannot find the path specified.
; class=Os (2)

Caused by:
    failed to make directory 'C:\Users\runneradmin\.cargo\registry/index\github.com-1ecc6299db9ec823': The system cannot find the path specified.
    ; class=Os (2)
Error: The process 'C:\Users\runneradmin\.cargo\bin\cargo.exe' failed with exit code 1

@obi1kenobi do you think it is a problem with the setup done by the action? Or might it be an issue with the cargo-semver-checks itself? I don't have a working Windows installation, so I didn't check it locally yet.

[obi1kenobi]

Or might it be an issue with the cargo-semver-checks itself?

This looks like a bug in cargo-semver-checks itself: it looks like it isn't properly constructing subdirectory names and using the wrong directory separator (the Unix / instead of the Windows \). Nice catch! Would you care to reproduce the problem in the cargo-semver-checks repo itself, and then debug and resolve it?

[obi1kenobi]

Excited to see this coming together nicely! 🎉

[mgr0dzicki]

I've found another (and the last) issue causing the tool to fail on Windows - the directory .cargo\registry\index does not exist at the beginning and therefore the creation of its subdirectory fails. The current workaround is 851a833.

@obi1kenobi, I've seen that you were successful in debugging the previous issue and fixing it in frewsxcv/rust-crates-index#94. Could you have a look at this problem too? I believe it shouldn't take you much time, as it is probably another simple fix in rust-crates-index code, which you've already worked with. Of course if you don't have enough time then I will take it ;) However I think I would first need to setup a virtual machine with Windows to debug it locally...

BTW, I'm back to work and hope to get v2 of the action finished soon!

[obi1kenobi]

I've found another (and the last) issue causing the tool to fail on Windows - the directory .cargo\registry\index does not exist at the beginning and therefore the creation of its subdirectory fails. The current workaround is 851a833.

This seems like it probably means cargo-semver-checks on Windows itself is broken too. Would you mind extracting a test case from this and adding it to the integration suite for cargo-semver-checks? I don't think you'd need a Windows VM for this.

Once we have that minimal test case, it will be easy to figure out whether it's something that cargo-semver-checks does incorrectly or whether we need an upstream fix in one of our dependencies, and if you'd prefer then I can take over from there.

[obi1kenobi]

Definitely moving in the right direction! Just a few "setting ourselves up for future success" suggestions.

[mgr0dzicki]

This seems like it probably means cargo-semver-checks on Windows itself is broken too. Would you mind extracting a test case from this and adding it to the integration suite for cargo-semver-checks? I don't think you'd need a Windows VM for this.

Sure, I'm currently working on that :).

[obi1kenobi]

Needs a bit of polish on the README, otherwise looks in pretty good shape!

What else does it need before it's ready to merge? I know we wanted to add baseline rustdoc caching, but we can do that in a separate PR and it'll be easier to review that way.

[mgr0dzicki]

Needs a bit of polish on the README, otherwise looks in pretty good shape!

What else does it need before it's ready to merge? I know we wanted to add baseline rustdoc caching, but we can do that in a separate PR and it'll be easier to review that way.

Thank you for all the comments and remarks, they are really valuable! I think the action is ready itself. I still want to add some workflow testing the action inputs (none of them is used at the moment). And then I think that polishing README and adding caching will be the only things remaining.

[obi1kenobi]

Sounds great! Thanks for all the awesome work you've been putting in!

[mgr0dzicki]

I polished the README, please take a look and let me know what do you think now :)

In the meanwhile, I'll start to work on rustdoc caching in a separate PR and notify you once it is ready for review.

[obi1kenobi]

Minor polish items, I think this should be the last iteration before we merge this 🚀

[obi1kenobi]

Just some lint-level nitpicks, otherwise looks ready to merge. I'm sure I only spotted them because I was looking at this in the PR diff view instead of an editor. consider doing a self-review of the PR to catch these yourself and reduce the number of back-and-forth iterations we need over tiny issues.

Once you've had a chance for that final tiny round of polish, just mark the PR as no longer a draft and I'd be happy to merge.

[mgr0dzicki]

Just some lint-level nitpicks, otherwise looks ready to merge. I'm sure I only spotted them because I was looking at this in the PR diff view instead of an editor. consider doing a self-review of the PR to catch these yourself and reduce the number of back-and-forth iterations we need over tiny issues.

I have to admit, I was using the editor and not looked at the PR diff view, my mistake. Sorry for all these issues! I try to double-check everything, but this PR is quite huge and I'm looking at it for a week now, so finding such mistakes becomes increasingly difficult...

I'll correct what you found in a moment and then self-review everything as you suggested.

[obi1kenobi]

Sorry for all these issues! I try to double-check everything, but this PR is quite huge and I'm looking at it for a week now, so finding such mistakes becomes increasingly difficult...

No worries and no need to apologize. Every experience is something we can learn from, and the top objective is the learning itself much more so than the artifact being produced in the PR itself.

I also feel the same way about the PR being huge and difficult to (re-)review over and over. In retrospect, both of us could probably have predicted this as a risk and structured it differently so we don't have to merge it all-or-nothing like this. We'll do better next time!

[mgr0dzicki]

Hope this is finally it... ;)

[obi1kenobi]

It looks good enough to merge, let's do that and move forward. I'm highlighting two small additional issues that we should try to fix in a subsequent PR.

[obi1kenobi]

Should this be required: true? Otherwise, what's the behavior if this input value isn't supplied?

Any time we make something optional, we're adding more modes of operation -- which means more things to test, and more things that can fail. Are we losing anything if we make this value required?

[mgr0dzicki]

My aim was to make the default behavior the same as for the checkout action, but you're right - since we don't use it, it's better to make the input required then wonder what the default behavior exactly is and try documenting it here.

[obi1kenobi]

The inputs here are generally great at documenting what the default behavior is when a required: false input isn't set. Let's document that behavior here as well.

[mgr0dzicki]

I've done this and the above one in #25