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

Update nextest to 0.9.57 #178

Closed
sunshowers opened this issue Aug 2, 2023 · 16 comments
Closed

Update nextest to 0.9.57 #178

sunshowers opened this issue Aug 2, 2023 · 16 comments

Comments

@sunshowers
Copy link
Contributor

Hi @taiki-e -- could you update nextest to 0.9.57? 0.9.56 has a bad bug and I needed to yank it. Thanks!

@sunshowers
Copy link
Contributor Author

(Separately, I'd like us to also figure out a way to quickly yank bad versions from the install-action.)

conorsch added a commit to penumbra-zone/penumbra that referenced this issue Aug 2, 2023
Upstream cargo-nextest shipped a broken version v0.9.56 [0], which is now
yanked, and superseded by v0.9.57. The GHA installer, however, hasn't
updated yet [1], so using a temporary workaround to unbreak CI.
The cargo-nextest maintainer has been lighting fast and extremely
communicative, so let's check back on the GHA issue later on, at which
point we can drop this empty file.

[skip ci]

[0] nextest-rs/nextest#926
[1] taiki-e/install-action#178
@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 3, 2023

Update is done, just waiting for a new release.

@NobodyXu NobodyXu closed this as completed Aug 3, 2023
@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 3, 2023

P.S. all updates are automatic created by GHA, so there's no need to opening an issue manually since we won't miss it.

You can have relax knowning that this repository will always keep them up to date!

Thank you for openin this issue.

@sunshowers
Copy link
Contributor Author

@NobodyXu, thanks. I think it's just a matter of urgency -- once an update has been yanked on crates.io (which was done within 5-10 minutes), it would be nice if the action no longer tried to fetch that version.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 3, 2023

@NobodyXu, thanks. I think it's just a matter of urgency -- once an update has been yanked on crates.io (which was done within 5-10 minutes), it would be nice if the action no longer tried to fetch that version.

Unfortunately the latest version info and checksum is hardcoded into the crate, so it requires a new release.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 3, 2023

In terms of this, cargo-binstall can indeed fetch the latest version since it does not hardcode anything.

@taiki-e
Copy link
Owner

taiki-e commented Aug 3, 2023

Published in 2.13.5.

@sunshowers
Copy link
Contributor Author

Thanks for all your hard work maintaining the action, Taiki!

@taiki-e
Copy link
Owner

taiki-e commented Aug 3, 2023

way to quickly yank bad versions from the install-action

I think it is possible to fetch the crates.io API during manifest generation to prevent the yanked version from being selected as a candidate for the latest or omitted version. This is not likely to very help this case, where the problem was resolved very quickly on the nextest side, since it is not reflected until the manifest update is released, but it would be helpful if the fix is not released quickly. It will also make the way described below more efficient.

I think it is also possible to fetch the crates.io API during installation and install the older version if the version is yanked (unless that version is explicitly specified). This could be implemented by adding a field to the manifest that makes fetching the older version easier, such as "previous_stable_version", and a field that indicates whether we want to fetch crates.io's API, such as "is_rust_crate", and code on the action side to fetch crates.io's API and select the version.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 3, 2023

way to quickly yank bad versions from the install-action

I think it is possible to fetch the crates.io API during manifest generation to prevent the yanked version from being selected as a candidate for the latest or omitted version. This is not likely to very help this case, where the problem was resolved very quickly on the nextest side, since it is not reflected until the manifest update is released, but it would be helpful if the fix is not released quickly. It will also make the way described below more efficient.

I think it is also possible to fetch the crates.io API during installation and install the older version if the version is yanked (unless that version is explicitly specified). This could be implemented by adding a field to the manifest that makes fetching the older version easier, such as "previous_stable_version", and a field that indicates whether we want to fetch crates.io's API, such as "is_rust_crate", and code on the action side to fetch crates.io's API and select the version.

In that scenario, won't it be easier to just use cargo-binstall which automatically does it for you?

The resolution speed for cargo-nextest isn't a problem for cargo-binstall because cargo-nextest overrides package.metadata.binstall in Cargo.toml.

We can fix the resolution speed for other crates supported in taiki-e/install-action by simply providing package.metadata.binstall.

@taiki-e
Copy link
Owner

taiki-e commented Aug 3, 2023

In that scenario, won't it be easier to just use cargo-binstall which automatically does it for you?

Even if we fall back to cargo-binstall, we need to know if the tool is rust crate and if the version is yanked to select whether to fallback to cargo-binstall, right? And if so, then at that point, the install-action side has most of the information needed to handle that without falling back to the cargo-binstall.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 3, 2023

Even if we fall back to cargo-binstall, we need to know if the tool is rust crate and if the version is yanked to select whether to fallback to cargo-binstall, right? And if so, then at that point, the install-action side has most of the information needed to handle that without falling back to the cargo-binstall.

I was thinking that perhaps we don't need to hard-code so many crates in taiki-e/install-action and can instead let cargo-binstall do the job for crates like cargo-nextest for every version.

@sunshowers
Copy link
Contributor Author

sunshowers commented Aug 3, 2023

I was thinking that perhaps we don't need to hard-code so many crates in taiki-e/install-action and can instead let cargo-binstall do the job for crates like cargo-nextest for every version.

I'd totally be open to this, as long as existing users don't break (note that nextest is currently defined as an alias for cargo-nextest). binstall already works with nextest.

@taiki-e
Copy link
Owner

taiki-e commented Aug 4, 2023

I was thinking that perhaps we don't need to hard-code so many crates in taiki-e/install-action and can instead let cargo-binstall do the job for crates like cargo-nextest for every version.

I'd totally be open to this, as long as existing users don't break (note that nextest is currently defined as an alias for cargo-nextest). binstall already works with nextest.

Well, this effectively means downgrading nextest's support status, so it will take longer to install, one of its security features will no longer be supported, and there is an increased chance of encountering network errors. There is no problem with the alias handling either way.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 4, 2023

Well, this effectively means downgrading nextest's support status, so it will take longer to install,

Yeah unfortunately it will be a little bit slower, since taiki-e/install-action would have to install cargo-binstall first.
But given that GHA has excellent network connection, I think the impact is going to be quite minimum.

Installing cargo-nextest using cargo-binstall should be quite fast since it overrides package.metadata.binstall so it won't take that long during resolution.

It will also support fallback to other target (gnu => musl, aarch64-apple-darwin => universal and x86_64, aarch64-pc-windows-msvc => x86_64).

I'd admit though that I am biased towards cargo-binstall.

one of its security features will no longer be supported,

If you are referring to checksum, then yeah this is a feature that isn't supported but planned cargo-bins/cargo-binstall#440 cargo-bins/cargo-binstall#1

One of the issue is that checksum and the pre-built artifacts are often stored together on GitHub Release, so it can be modified.

We are thinking about supporting sigstore which use public-private key-pair and other schema.

and there is an increased chance of encountering network errors. There is no problem with the alias handling either way.

Yes, it will certainly increase the probability of network errors due to target fallback.

@sunshowers
Copy link
Contributor Author

Yeah, it would be ideal to make a data-driven decision about the rate of network failures (compared to eg the disruption from wide-scale outages of the sort GH has regularly) but I don't think we have that data.

I think on balance I think I lean towards binstall, and I wouldn't necessarily consider it a downgrade personally.

(The checksum issues should eventually be solved in binstall I agree, and I'm excited for whatever sigstore-based solution they come up with)

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

No branches or pull requests

3 participants