Skip to content

ci: fix flaky binstall-check job#2170

Merged
thomas-zahner merged 2 commits into
masterfrom
fix-binstall-check-ci
Apr 27, 2026
Merged

ci: fix flaky binstall-check job#2170
thomas-zahner merged 2 commits into
masterfrom
fix-binstall-check-ci

Conversation

@mre

@mre mre commented Apr 25, 2026

Copy link
Copy Markdown
Member

The binstall-check job from #2165 fails for two reasons:

  • Unauthenticated api.github.com requests get 403 under contention. Fix by passing GITHUB_TOKEN.
  • Release-plz PRs bump the in-tree version to one that is not published yet, so binstall (with --manifest-path .) cannot find a matching release. Drop the flag so binstall resolves the latest published version, and skip the job on release-plz branches as a safety net.

Should unblock #2169 and future release-plz PRs.

mre added 2 commits April 25, 2026 16:40
Two failure modes were causing red CI on unrelated PRs and on every
release-plz PR:

- Unauthenticated api.github.com requests hit 403 under contention.
  Pass GITHUB_TOKEN so binstall's GitHub API calls are authenticated.
- Release PRs bump the in-tree version to one that is not published
  yet, so '--manifest-path .' made binstall look for a non-existent
  release. Drop the flag so binstall resolves the latest published
  version, and skip the job entirely on release-plz branches.
Dropping it caused binstall to resolve the latest published release
(0.24.1), whose archive still has the pre-#2165 layout, so the job
fails with 'lychee bin not found'. Using the in-tree manifest is the
whole point: it validates the metadata before the next release.

@katrinafyi katrinafyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing :)

Yeah, manifest path is needed but it's still an imperfect test. It's installing the latest release archive (which is likely older than master) using the binstall metadata from master.

I don't know if there's a good way around this.. maybe we can try to install a nightly release archive? It would still be 24 hours behind, but it would let us test with the upcoming release binaries.

@thomas-zahner

thomas-zahner commented Apr 27, 2026

Copy link
Copy Markdown
Member

Yeah, manifest path is needed but it's still an imperfect test. It's installing the latest release archive (which is likely older than master) using the binstall metadata from master.

I think this is fine. We basically want to lint/test if binstall can parse our metadata. The archive contents are not really that relevant. I found out that it's possible to specify --git "file:///$(pwd)" but that also doesn't seem to be really helpful either.

In other words, I don't think we would detect any more errors if we test against nightly instead of latest release with --manifest-path . as the archive contents should contain a valid binary anyways. But maybe I'm missing something.

Going to merge now. If there actually still is an improvement to be made it can be done in a fallow up PR.

@thomas-zahner thomas-zahner merged commit c68af26 into master Apr 27, 2026
14 of 15 checks passed
@thomas-zahner thomas-zahner deleted the fix-binstall-check-ci branch April 27, 2026 10:01
@katrinafyi

Copy link
Copy Markdown
Member

With testing nightly, the purpose would be to guard against changes in the release archive structure and to catch this before the official release is pushed. But it's not super urgent and there's probably not many more release changes coming up.

@thomas-zahner

thomas-zahner commented May 1, 2026

Copy link
Copy Markdown
Member

With v0.24.2 the job fails :/ but maybe rightfully so?

Edit: huh, seems to be fixed after rerunning the job 🤷

@katrinafyi

Copy link
Copy Markdown
Member

It might be using the version from Cargo.toml trying to install the new version before the binaries are uploaded. Just a guess though.

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.

3 participants