taiki-e / cargo-hack

Cargo subcommand to provide various options useful for testing and continuous integration.
Apache License 2.0
628 stars 28 forks source link

fix: Respect --locked flag #236

Closed epage closed 8 months ago

epage commented 8 months ago

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

This is part of #234

taiki-e commented 8 months ago

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. https://github.com/taiki-e/cargo-hack/blob/6350a4950ab5aec7cbe0e24d8259edc37f5043cb/tests/fixtures/namespaced_features/Cargo.toml#L11-L12
  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 commented 8 months ago

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 commented 8 months ago

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. https://github.com/taiki-e/cargo-hack/blob/6350a4950ab5aec7cbe0e24d8259edc37f5043cb/Cargo.toml#L62

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!

epage commented 8 months ago

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 commented 8 months ago

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?

taiki-e commented 8 months ago

Published in 0.6.20.

epage commented 8 months ago

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 commented 8 months ago

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 https://github.com/taiki-e/install-action/issues/348#issuecomment-1909197273

taiki-e commented 8 months ago

This is now included in the latest version of install-action (https://github.com/taiki-e/install-action/pull/377).