Skip to content

Comments

chore: RM dry run workflow#1690

Merged
sergerad merged 1 commit intonextfrom
sergerad-rm-dry-run
Feb 24, 2026
Merged

chore: RM dry run workflow#1690
sergerad merged 1 commit intonextfrom
sergerad-rm-dry-run

Conversation

@sergerad
Copy link
Collaborator

@sergerad sergerad commented Feb 19, 2026

Seems outdated (next failing on missing script). We can reinstate it when we are ready to update it. Would like the repo to be green building.

@sergerad sergerad added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Feb 19, 2026
Copy link
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my preference would be for the release mechanism to have this built-in as a prerequisite.

As in, the release first does a dry-run of everything it wants and needs to check, before performing the actual release.

Its broken now because I removed the check-msrv script in #1673, but it usually also failed prior to that. This is because we often have our protocol pinned to next or some git branch, and that fails the publishing dry-run check.

@bobbinth
Copy link
Contributor

As in, the release first does a dry-run of everything it wants and needs to check, before performing the actual release.

I think the downside of this is that we won't know until release time if the code is in "publishable" state (previously, before we had this, I'd frequently find out at release time that something isn't working, and it'd take me sometimes a few hours to fix things up that should have been fixed in some old PRs).

It would be great to have a check that w/e is in next can actually be published to crates.io. If the current check is broken, we can remove it, but I'd prefer to add something like that in the near future.

@Mirko-von-Leipzig
Copy link
Collaborator

It would be great to have a check that w/e is in next can actually be published to crates.io. If the current check is broken, we can remove it, but I'd prefer to add something like that in the near future.

Just to be sure -- this workflow took 45 minutes per run when it worked. Which is fairly unreasonable to run on every commit to next; though maybe its fine. Though that its usually red due to unpublished downstream dependencies makes it rather less than useful imo (i.e. whenever we pin to protocol next).

I agree we want something but I'm not sure that the above is the way.

My suggestion with the release pre-checks is really more to ensure that everything is ready for publishing e.g. docker works, cargo publish works, debian packages work, integration tests pass etc. And then only once everything is gathered and one clicks release does everything get uploaded with fairly high confidence that it all works. This is orthogonal to your request imo -- and I'm unsure how to resolve that.

@bobbinth
Copy link
Contributor

Though that its usually red due to unpublished downstream dependencies makes it rather less than useful imo (i.e. whenever we pin to protocol next).

Do you mean "upstream" rather than "downstream" dependencies - i.e., because we use git dependencies for miden-base?

Which is fairly unreasonable to run on every commit to next

If the runtime is the only issue, I think we can run it once a day - or maybe even once a week.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: it's fine to remove the script, but it will impose more work on the releaser. We could throw away the cargo publish --dry-run part which seems to be the main problem and consider keeping the check-msrv part.

In detail:

  • The CI failure for the missing script error was caused by PR #1673. That commit removed scripts/check-msrv.sh but only updated the nightly.yml workflow to use cargo msrv directly instead of the script.
  • The CI failure for cargo publish --dry-run and cargo msrv are different issues. The publish-dry-run workflow happened to run both checks, but they're orthogonal.
  • I analyzed all 128 commits between #1509 (script setup) and #1673.
    Gist here.
    • PASSED: 19 commits - The workflow completed successfully
    • PUBLISH_GIT_DEP failures: 77 commits - These failed due to git dependencies in Cargo.toml that aren't allowed by crates.io
    • CANCELLED: 19 commits - The workflow was cancelled (usually by a newer commit)
    • NO_CI_RUN: 30 commits - No CI run was found for these commits
    • MISSING_SCRIPT: 2 commits - Failed because scripts/check-msrv.sh was missing (after commit 21a84c5)
  • To the best of my knowledge, the MSRV check portion of the job has not yet failed.
  • The average runtime of the whole release --dry-run when this passes is indeed ~45 min.
  • With that said, the average time between two commits in these same 128 commits is 416 minutes, the job's runtime is ~33rd percentile of the inter-commit duration.

@Mirko-von-Leipzig
Copy link
Collaborator

Mirko-von-Leipzig commented Feb 23, 2026

Do you mean "upstream" rather than "downstream" dependencies - i.e., because we use git dependencies for miden-base?

Ah yes "upstream". cargo publish cannot work when with git deps because it of course requires that all non-workspace dependencies already exist on crates.io. And since that's the majority of the time, it makes the check somewhat dead in the water.

The average runtime of the whole release --dry-run when this passes is indeed ~45 min.
With that said, the average time between two commits in these same 128 commits is 416 minutes, the job's runtime is ~33rd percentile of the inter-commit duration.

CI checks must provide fast feedback. Long run times waste dev time because they wait for things to be green. Ideally things would be ~5mins at most; anything above ~10min is effectively useless for regularly CI imo.

I'm happy with these things being part of a pre-release check. But as something that always runs on every PR and every commit is problematic. I'm also not really happy with having loose script files. Devs should know how to use their tools; and in 90% of cases the script doesn't actually do what I want; CI and dev runs are different use case wise and having a script obscures what's actually happening.

I've moved most of the "expensive" checks to nightly. We still get feedback, but its not so critical that these are always run on every commit and merge.

Personally I think having such checks run once a day is sufficient -- msrv failures are not critical imo since we're a binary/application and not a library. Its mostly a nice-for-dev purposes thing and shouldn't block a release.

Having daily checks and then a more diligent release action is both more comprehensive and easier to manage imo. As in, we have a release workflow which first does a dry-run of everything (not just publishing), and then drafts the github release only once things succeed. Once you click release on github, that the workflow publishes the dry-run artifacts (docker, debian packages, cargo crates).

@huitseeker
Copy link
Contributor

Nightly sounds like a great way to get signal without hindrance.

@sergerad sergerad merged commit d970008 into next Feb 24, 2026
19 of 20 checks passed
@sergerad sergerad deleted the sergerad-rm-dry-run branch February 24, 2026 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants