Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove --allow-dirty option … #702

Closed
wants to merge 2 commits into from

Conversation

Olf0
Copy link

@Olf0 Olf0 commented May 10, 2024

… from cargo publish calls in the CI/CD workflows, because this option prevents creating a .cargo_vcs_info.json file. This should (really) close rust-lang/crates.io#8551 , see there for details.

Signed-off-by: olf [email protected]

@Olf0 Olf0 requested a review from a team as a code owner May 10, 2024 21:31
@Olf0 Olf0 changed the title Remove --allow-dirty option from cargo publish calls … Remove --allow-dirty option … May 10, 2024
@Olf0
Copy link
Author

Olf0 commented May 10, 2024

Note that another solution would be to change the CONTRIBUTING link in the README.md to an absolute one.

Currently my thinking is that omitting --allow-dirty is the preferable way, but if there is a proper reason why the crate publish call should succeed even if the repository is in an unclean state, altering the link proper is an alternative.

@Olf0
Copy link
Author

Olf0 commented May 11, 2024

Please squash-merge, then you should obtain the "Signed-off-by:" line your DCO demands by the PR description.
As I performed this minimalistic change at GitHub's web-frontend, I have no local clone of your git repo and see no possibility to rectify this retrospectively (without much effort for no real gain).

@lfrancke
Copy link
Contributor

Thank you for the contribution.
I'm not an expert at publishing crates but your reasoning sounds totally sane. allow-dirty does not sound like something we want to have.

I do not feel comfortable merging this as-is because of the DCO.
I do understand that it is extra effort for you. But if you do reconsider you would find the steps to add the DCO here: https://github.com/CycloneDX/cyclonedx-rust-cargo/pull/702/checks?check_run_id=24839175861

@Shnatsel
Copy link
Contributor

I assume the flag is there for a reason. We always wanted it gone, but it wasn't clear why it was there in the first place, and whether the workflow would fail without this flag.

I believe the workflow is going to fail with the proposed patch because cargo publish is run before git commit, so there will be uncommitted changes.

@Olf0
Copy link
Author

Olf0 commented May 25, 2024

I assume the flag is there for a reason. We always wanted it gone, but it wasn't clear why it was there in the first place, and whether the workflow would fail without this flag.

These statements come a bit surprising to me from a project member. IMO, then it is overdue to care about it, especially because it breaks something at crates.io (an aspect you did not touch, though it is the principal one).

I believe the workflow is going to fail with the proposed patch because cargo publish is run before git commit, so there will be uncommitted changes.

Could be … or not. Testing would likely reveal if that is the case. Because I do not use Rust and consequently neither this or any other Rust crate, I have not set up an environment for testing and do not plan to do that.

Actually I do not assume that the CI/CD workflow fails, only the tagged state of git repo is not identical to the repo state from which the crate was packaged: The latter lacks the output of a cargo package as part of the cargo publish call. I wonder if that really matters, but have no idea.

A "checks & balance" question is: Is the output of the cargo package (as part of the cargo publish in the step Publish) relevant for the subsequent and final step Configure git and add files of the CI job?
Looking at what is actually committed then, the answer seems to be "No": The committed changes are generated a lot earlier in the step Cargo bump, hence it may be possible to commit them before the cargo publish call.
But ultimately I abstained from further following this route, in order to stay on a safe track.

Additionally I was considering to suggest to eliminate the superfluous step Build one time, for sanity, because its cargo build is also performed by cargo package (as part of the cargo publish) in the subsequent step Publish, before anything is published (i.e. nothing is published if building fails).
But ultimately it might make sense to alter this step to perform a cargo package run with --allow-dirty, which also rewrites the TOML file etc., then to commit and tag, and lastly publish by cargo publish with --allow-dirty: This appears to be the safest route, aside the "quick & dirty" option to utilise an absolute web-link (instead of an relative one).
Unfortunately cargo publish always includes a cargo package run: There is not command line option to publish an extant .crate file.

@Olf0
Copy link
Author

Olf0 commented May 25, 2024

Superseded by PR #719.

@Olf0 Olf0 closed this May 25, 2024
@Olf0 Olf0 deleted the remove_--allow-dirty branch May 25, 2024 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken CONTRIBUTING link for crate cargo-cyclonedx
3 participants