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

fix: Respect --locked flag #236

Merged
merged 3 commits into from
Feb 21, 2024
Merged

fix: Respect --locked flag #236

merged 3 commits into from
Feb 21, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Feb 19, 2024

When using --rust-version, the lockfile can be blown away. While that is a problem in of its own,
this focuses on an incremental step of not blowing it away if --locked is used.

Some points that were unclear in implementation

  • Whether --locked should show up in --help
  • Where to put in --help (cargo puts it among the manifest options)
  • Ordering of everything (seems struct declaration and initialization is not kept in sync

This is part of #234

@taiki-e
Copy link
Owner

taiki-e commented Feb 20, 2024

Thanks!

  • Whether --locked should show up in --help
  • Where to put in --help (cargo puts it among the manifest options)

We also show some of the Cargo options in help, such as manifest-path, so I think this PR is fine as it is. That said, the current one is not that consistent, so ideally we might want to show only cargo-hack-specific flags or other Cargo flags as well.

  • Ordering of everything (seems struct declaration and initialization is not kept in sync

I think clippy has a lint for this, but they don't seem to be complaining...?
Anyway, I agree that it is easier to understand if they are in definition order, unless there are value dependencies that cannot be reordered.

  • How to test as this requires the registry and I didn't see precedence for that

I have not tested it, but the following might work:

  1. Create a test crate that depends on easytime (that the latest version cannot be compiled with the old rustc) with the normal easytime = "0.2" requirements.
    # easytime 0.2.6 requires Rust 1.58
    easytime = { version = "=0.2.5", optional = true, default-features = false }
  2. Downgrade the easytime version to 0.2.5 using cargo update (and commit its lockfile).
  3. Set MSRV of that test crate to pre-1.58.
  4. Run cargo-hack on that test crate with --rust-version or --version-range.

@epage
Copy link
Contributor Author

epage commented Feb 20, 2024

I think clippy has a lint for this, but they don't seem to be complaining...?
Anyway, I agree that it is easier to understand if they are in definition order, unless there are value dependencies that cannot be reordered.

Looks to default to allow.

https://rust-lang.github.io/rust-clippy/master/index.html#/inconsistent_struct_constructor

I have not tested it, but the following might work:

More so I meant I hadn't seen registry packages used and they can get messy (more chance of network disruptions, changes from new packages being published, etc).

If you're fine with it, I can add it.

@taiki-e
Copy link
Owner

taiki-e commented Feb 20, 2024

Looks to default to allow.

https://rust-lang.github.io/rust-clippy/master/index.html#/inconsistent_struct_constructor

Hmm. It is pedantic lint, so it should already be set to warn.

pedantic = "warn"

More so I meant I hadn't seen registry packages used and they can get messy (more chance of network disruptions, changes from new packages being published, etc).

Thanks for the clarification. As for the former, I think it would be fine. Even if it is very bad it can be handled using flaky_test crate or similar. As for the latter, we want to test if it is respected by specifying an older version, so we probably don't have to worry about what happens with the newer version.

If you're fine with it, I can add it.

That would be great!

When using `--rust-version`, the lockfile can be blown away.
While that is a problem in of its own,
this focuses on an incremental step of not blowing it away if `--locked`
is used.

This is part of taiki-e#234
@epage
Copy link
Contributor Author

epage commented Feb 20, 2024

Realized the problem with the lint

all fields are shorthand

Thats an annoying limitation. Its to avoid when order of field initialization is important, like if borrows are involved.

See rust-lang/rust-clippy#11846

@epage
Copy link
Contributor Author

epage commented Feb 20, 2024

We also show some of the Cargo options in help, such as manifest-path, so I think this PR is fine as it is. That said, the current one is not that consistent, so ideally we might want to show only cargo-hack-specific flags or other Cargo flags as well.

It wasn't too clear to me if there is something I need to do in response to this. You said the current one isn't consistent. I tried to make it consistent (and my latest force-push makes it more consistent). Is there something more I should be doing? Or is this only with the inconsistency on when cargo flags are shown more generally and separate?

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@taiki-e taiki-e merged commit 9e01938 into taiki-e:main Feb 21, 2024
27 checks passed
@taiki-e
Copy link
Owner

taiki-e commented Feb 21, 2024

Published in 0.6.20.

@epage epage deleted the locked branch February 21, 2024 02:20
@epage
Copy link
Contributor Author

epage commented Feb 21, 2024

Thanks!

Not seeing the job that is supposed to update the manifest for install-action (e.g. taiki-e/install-action#374). Where can I track the status of that?

@taiki-e
Copy link
Owner

taiki-e commented Feb 21, 2024

Not seeing the job that is supposed to update the manifest for install-action (e.g. taiki-e/install-action#374). Where can I track the status of that?

It is usually checked automatically every 3 hours (last check was 2 hours ago). It can also be triggered manually, I just triggered it manually now (https://github.com/taiki-e/install-action/actions/runs/7982772416/job/21798009113).

See also taiki-e/install-action#348 (comment)

@taiki-e
Copy link
Owner

taiki-e commented Feb 21, 2024

This is now included in the latest version of install-action (taiki-e/install-action#377).

epage added a commit to epage/cargo that referenced this pull request Mar 3, 2024
As a hack in cargo-hack, it doesn't respect lockfiles when doing MSRV
testing unless `--locked` is passed in.
This adds that so we make sure we don't run into problems with newer,
MSRV-imcompatible dependencies come out that break our build.

See
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/gix-ref.20CI.20error/near/423319798
- taiki-e/cargo-hack#234
- taiki-e/cargo-hack#236
bors added a commit to rust-lang/cargo that referenced this pull request Mar 3, 2024
chore(ci): Ensure lockfile is respected during MSRV testing

As a hack in cargo-hack, it doesn't respect lockfiles when doing MSRV testing unless `--locked` is passed in.
This adds that so we make sure we don't run into problems with newer, MSRV-imcompatible dependencies come out that break our build.

See
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/gix-ref.20CI.20error/near/423319798
- taiki-e/cargo-hack#234
- taiki-e/cargo-hack#236
charmitro pushed a commit to charmitro/cargo that referenced this pull request Sep 13, 2024
As a hack in cargo-hack, it doesn't respect lockfiles when doing MSRV
testing unless `--locked` is passed in.
This adds that so we make sure we don't run into problems with newer,
MSRV-imcompatible dependencies come out that break our build.

See
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/gix-ref.20CI.20error/near/423319798
- taiki-e/cargo-hack#234
- taiki-e/cargo-hack#236
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.

2 participants