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

Add lint to explicitly set resolver in a virtual workspace #10112

Closed
TheButlah opened this issue Nov 22, 2021 · 13 comments · Fixed by #10910
Closed

Add lint to explicitly set resolver in a virtual workspace #10112

TheButlah opened this issue Nov 22, 2021 · 13 comments · Fixed by #10910
Labels
A-features2 Area: issues specifically related to the v2 feature resolver A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@TheButlah
Copy link
Contributor

TheButlah commented Nov 22, 2021

Problem

Because the resolver is a global setting in workspaces, and workspaces don't have edition settings, I believe that it is very easy to accidentally use the old resolver in workspaces where you intended on using the new resolver. Because I didn't know about the resolver being global, I just assumed I was using the new one since my crate's edition said "2021" and it caused me to spend the last 4 days trying to fix my builds.

Proposed Solution

I think that having a lint in cargo that says "hey you should explicity set your resolver" would be merited. Because otherwise everyone is going to be using the old resolver all the time, perhaps even without realizing.

Notes

See also rust-lang/rust#90148 and #9996

@TheButlah TheButlah added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Nov 22, 2021
@ehuss ehuss added A-features2 Area: issues specifically related to the v2 feature resolver A-workspaces Area: workspaces labels Dec 6, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented Dec 10, 2021

There is an issue of people who are not using resolver so that their workspace is compatible with older Cargos. This becomes less of an issue overtime, but is still a thing. So maybe we worn on a 2021 crate being built in a workspace that did not set resolver.

@TheButlah
Copy link
Contributor Author

TheButlah commented Dec 10, 2021

Crates != workspace though, that's why the issue is here to begin with. It's not sufficient to set resolver =2 in the crates cargo.toml, it has to be done in the workspace Cargo.toml

I'm advocating for a lint that suggests explicitly setting the resolver (even if resolver = 1) in the workspace Cargo.toml

@TheButlah TheButlah changed the title Add resolver=2 lint Add lint to explicitly set resolver Dec 10, 2021
@johnkjellberg
Copy link

As a minimum, I think it would make sense to warn (or generate a error) if a crate explicitly sets resolver = 2, but it uses resolver v1 due a workspace in the root without a resolver set/resolver set to v1.

@DusterTheFirst
Copy link

Having the default workspace resolver as v1 creates confusion in workspaces with all crates using edition = "2021". Telling the user that the resolver might not match their expectation can save days of troubleshooting weird dependency problems as in my and @TheButlah's cases.

@Muscraft
Copy link
Member

Part of workspace inheritance is the ability for a member to inherit the edition from its workspace. #10587 was opened to discuss if setting the edition in [workspace.package] should imply the resolver version for the workspace if one is not set. This might help mitigate this issue for users of workspace inheritance. If anyone has thoughts on this please add them to the issue.

@Nemo157
Copy link
Member

Nemo157 commented Jul 28, 2022

To extract some discussion from #10910:

The lint I propose is to emit a warning any time one of the members of a workspace has a different resolver to the workspace itself. This is necessary for binaries because developers will expect that when they cargo build locally in their workspace, then cargo publish && cargo install the same feature-resolution algorithm will be used. For libraries it's less important since their resolver is set by the root of whatever build-tree they're in, but there is one important usecase that builds with the library as the root: docs.rs. There were issues building the docs for axum despite them having a local CI test for the docs.

Generally fixing this lint is just adding workspace.resolver = "2" (or workspace.edition = "2021" if #10587 is implemented) and making sure you have updated all your crates to edition 2021. I would be surprised if there are any workspaces that want to have disparate editions longterm, so if there is another version of the resolver implied by a future edition then you would just have to update the resolver version at the same time you update all your editions.

The biggest downside I see is that this will be instantaneously very noisy. I'm certain that 99% of virtual workspaces are currently setup incorrectly since I have never seen workspace.resolver being used.

@epage
Copy link
Contributor

epage commented Jul 28, 2022

This is necessary for binaries because developers will expect that when they cargo build locally in their workspace, then cargo publish && cargo install the same feature-resolution algorithm will be used.

@Nemo157 do you have a reproduction case for this local and published resolution being different?

When we clean up the manifest for publish, we overwrite the package.resolver with workspace.resolver (after all defaulting as been applied), see https://github.com/rust-lang/cargo/blob/master/src/cargo/util/toml/mod.rs#L1345. That should cause the published package's behavior to be the same as local development so long as the package is always within a workspace (e.g. cargo isn't always in a workspace so it can get inconsistent results depending on how you develop on it locally and publish is just consistent with which case cargo was published from)

@Nemo157
Copy link
Member

Nemo157 commented Jul 28, 2022

ResolverBehavior::V1.to_manifest() == None. So this will only upgrade an edition = "2015" crate inside a resolver = "2" workspace to use package.resolver = "2", it will not downgrade a crate to use the old resolver. The linked issue from axum is exactly this case, within their repository they were using resolver = "1" because it was in a virtual workspace and did not override it, they then published a crate using edition = "2021" so it implicitly used resolver = "2".

@epage
Copy link
Contributor

epage commented Jul 28, 2022

@ehuss looks like you added all of this in #8129. Happen to remember if its intentional that TomlManifest::prepare_for_publish will only put the workspace's resolver version into the published manifest if its not v1 and cause an edition="2018" package to build differently locally than with cargo install from the registry?

If its not intentional for local and registry packages to build differently, thoughts on if changing this is breaking or not? To me, it doesn't seem breaking as it only affects newly published packages, it would make newly published packages avoid known failure cases, and I can't imagine anyone is relying on this behavior (consistently installing the same package and would be negatively impacted by the change in dependencies chosen).

@ehuss
Copy link
Contributor

ehuss commented Jul 29, 2022

No, I don't think that behavior was intentional. I think it would be good to fix it. There is some risk that it might impact someone, but I think the risk is low enough that it shouldn't be too much of an issue.

@ehuss
Copy link
Contributor

ehuss commented Jul 29, 2022

I haven't looked too closely, but I think #10910 is perhaps a bit too aggressive.

I might suggest a warning that triggers if: it is a virtual workspace AND it does not specify the resolver AND all members have an edition >= 2021. The solution to remove the warning is to specify workspace.resolver. That seems like the be the scenario that most people are running into, and shouldn't be too noisy for larger workspaces.

It's really unfortunate to force people to put boilerplate in the [workspace] definition, and I suspect the warning will be annoying for some. I can't think of a better way to avoid it.

I would be surprised if there are any workspaces that want to have disparate editions longterm

I do not think that is too unusual.

@Nemo157
Copy link
Member

Nemo157 commented Jul 29, 2022

I think it would be good to fix it

Great, that should fix all the actual build failures I can see coming from this.

That seems like the be the scenario that most people are running into, and shouldn't be too noisy for larger workspaces.

That's the scenario currently, but if we get a future edition 2030 which implies resolver 3 we'll have the same issue again, people upgrade their crates to edition 2030 and don't realise their workspace is still set to resolver 2. Though that may be mitigated by #10587 if people move to specifying workspace.edition instead of per-crate edition.

@epage
Copy link
Contributor

epage commented Jul 29, 2022

but if we get a future edition 2030 which implies resolver 3

While normally I don't like punting on problems as people are likely to forget about them, in this case I think the landscape could change significantly enough between now and then that a solution that negatively affects the now isn't worth it. For example, we've talked about adding lint control to cargo, giving users control over lints which opens the door to us being noisier in the future.

bors added a commit that referenced this issue Jul 29, 2022
Override to resolver=1 in published package

As discussed in #10112 this avoids inconsistent dependency resolution in development and packaged builds when an edition 2021 crate is published from a workspace using the default resolver=1.
@ehuss ehuss changed the title Add lint to explicitly set resolver Add lint to explicitly set resolver in a virtual workspace May 9, 2023
@bors bors closed this as completed in d08c587 May 26, 2023
fornwall added a commit to fornwall/advent-of-code that referenced this issue Jun 12, 2023
rwestphal added a commit to holo-routing/holo that referenced this issue Jun 18, 2023
...just do this to get rid of a new nightly compiler warning. This
change shouldn't impact Holo in any meaningful way.

References:
* rust-lang/cargo#10112
* https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2

Signed-off-by: Renato Westphal <[email protected]>
xxchan added a commit to xxchan/opendal that referenced this issue Jul 3, 2023
warning: some crates are on edition 2021 which defaults to `resolver = "2"`, but virtual workspaces default to `resolver = "1"`
note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest
note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest

As a library, this isn't important as end-user application decides the resolver finally. Just remove the warning.

ref:
- rust-lang/cargo#10112
- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
Xuanwo pushed a commit to apache/opendal that referenced this issue Jul 4, 2023
warning: some crates are on edition 2021 which defaults to `resolver = "2"`, but virtual workspaces default to `resolver = "1"`
note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest
note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest

As a library, this isn't important as end-user application decides the resolver finally. Just remove the warning.

ref:
- rust-lang/cargo#10112
- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
ariel-miculas added a commit to ariel-miculas/puzzlefs that referenced this issue Jul 18, 2023
We cannot set the rust edition inside the virtual workspace, so we need
to set the resolver field. For the 2021 edition, resolver version 2 is
recommended.

See:
https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
rust-lang/cargo#10112

Signed-off-by: Ariel Miculas <[email protected]>
etungsten added a commit to etungsten/bottlerocket that referenced this issue Aug 25, 2023
This gets rid of a warning that comes with rust 1.72.0. This does not
impact any of our existing crates since they're all on edition 2021.

See rust-lang/cargo#10112
etungsten added a commit to etungsten/bottlerocket that referenced this issue Aug 31, 2023
This gets rid of a warning that comes with rust 1.72.0. This does not
impact any of our workspaces since we've always been defaulted to using
resolver version 1. We can't use resolver v2 yet since that breaks the
'cargo make build-package' task.

See rust-lang/cargo#10112 for more details.
webern pushed a commit to webern/twoliter that referenced this issue Sep 6, 2023
This gets rid of a warning that comes with rust 1.72.0. This does not
impact any of our existing crates since they're all on edition 2021.

See rust-lang/cargo#10112
webern pushed a commit to webern/twoliter that referenced this issue Sep 6, 2023
This gets rid of a warning that comes with rust 1.72.0. This does not
impact any of our workspaces since we've always been defaulted to using
resolver version 1. We can't use resolver v2 yet since that breaks the
'cargo make build-package' task.

See rust-lang/cargo#10112 for more details.
brandonweeks added a commit to google/native-pkcs11 that referenced this issue Sep 7, 2023
```
warning: some crates are on edition 2021 which defaults to `resolver = "2"`, but virtual workspaces default to `resolver = "1"`
note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest
note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest
```

Context: rust-lang/cargo#10112
brandonweeks added a commit to google/native-pkcs11 that referenced this issue Sep 7, 2023
```
warning: some crates are on edition 2021 which defaults to `resolver = "2"`, but virtual workspaces default to `resolver = "1"`
note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest
note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest
```

Context: rust-lang/cargo#10112
Ironedde pushed a commit to Ironedde/mcan-clone that referenced this issue Oct 20, 2023
Resolver need to be set on workspace level.

See rust-lang/cargo#10112
Ironedde pushed a commit to Ironedde/mcan-clone that referenced this issue Oct 22, 2023
Resolver need to be set on workspace level.

See rust-lang/cargo#10112
rwestphal added a commit to holo-routing/holo that referenced this issue Jan 25, 2024
...just do this to get rid of a new nightly compiler warning. This
change shouldn't impact Holo in any meaningful way.

References:
* rust-lang/cargo#10112
* https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2

Signed-off-by: Renato Westphal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features2 Area: issues specifically related to the v2 feature resolver A-workspaces Area: workspaces C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants