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

Infer resolver version from workspace edition #10587

Open
ehuss opened this issue Apr 21, 2022 · 14 comments
Open

Infer resolver version from workspace edition #10587

ehuss opened this issue Apr 21, 2022 · 14 comments
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2022

It would be very helpful to set the resolver version in a virtual workspace based on the edition field being added in RFC 2906. This has been requested several times and has caused some confusion for some users.

That is, if you have:

[workspace]
edition = "2021"
members = ["foo"]

Then this will default workspace.resolver = "2"

The logic for determining the version is here. It should also check the workspace edition. This should also be gated on workspace-inheritance on nightly.

@ehuss ehuss added E-easy Experience: Easy A-workspace-inheritance Area: workspace inheritance RFC 2906 labels Apr 21, 2022
@Rustin170506
Copy link
Member

@rustbot claim

@Muscraft
Copy link
Member

@hi-rustin If you don't mind I would like to claim this. I want to add it to the RFC as something to do before we try to stabilize.

@Rustin170506
Copy link
Member

I want to add it to the RFC as something to do before we try to stabilize.

I don't quite understand what you mean?

@Muscraft
Copy link
Member

I'm currently working on the RFC for workspace inheritance and would like to add this issue to the tracking issue as something to do before trying to stabilize it.

@Rustin170506
Copy link
Member

You mean we don't need to implement it now? (Or is there something blocking it?)

The logic for determining the version is here. It should also check the workspace edition. This should also be gated on workspace-inheritance on nightly.

This confuses me 😖 I thought we were going to make it happen now.

Unassign myself.

@Rustin170506 Rustin170506 removed their assignment Apr 21, 2022
@Rustin170506
Copy link
Member

If it is now acceptable and achievable. Then I'm happy to implement it :(

@Muscraft
Copy link
Member

That's my bad for being confusing. I was just trying to say that I was just going to add it to my todo list before trying for stabilization of workspace inheritance.

If it's something you want to work on you are more than welcome to. There shouldn't be any blockers on it.

@epage
Copy link
Contributor

epage commented Apr 21, 2022

@ehuss my comment in #5784 used the workspace.edition = "2018" syntax because that is what is in the inheritance RFC, the current inheritance implementation has the syntax as workspace.package.edition = "2018". Should this issue reflect whatever syntax is stablized for inheritance or should it use workspace.edition, regardless of what inheritance does?

Background: there is unresolved thread from the RFC that I think needs revisiting and so #10564 changed the inheritable syntax to workspace.package.edition = "2018" as a proposed change for stablization as recorded in #8415.

@Muscraft
Copy link
Member

Muscraft commented Apr 21, 2022

Another thing to consider is having workspace.edition = true caused #10586 to some degree. By having things to inherit under workspace.package.{key} or [workspace.package], it makes it harder to mix up [workspace] fields and [package] fields.

@ehuss ehuss removed the E-easy Experience: Easy label Apr 21, 2022
@ehuss
Copy link
Contributor Author

ehuss commented Apr 21, 2022

Ah, sorry for the confusion. I probably should not have marked this with an E tag as this is indeed part of the RFC 2906 work. @hi-rustin, it looks like you have several open issues assigned to you. I recommend trying to finish some of those off before claiming additional issues. I appreciate the interest in helping, but claiming too many issues at the same time can make coordination more difficult.

@epage Sorry, I did not see that had changed. That indeed puts a wrinkle in this. I wouldn't want those inheritance fields to have no intrinsic meaning, except for edition. But I wouldn't want to add workspace.edition by itself with no meaning other than changing the resolver (or adding implicit inheritance).

Unless anyone has particular ideas on how to deal with that, I'm leaning towards just closing this.

@epage
Copy link
Contributor

epage commented Apr 21, 2022

Unless anyone has particular ideas on how to deal with that, I'm leaning towards just closing this.

Let's at least leave this open until 2906 is stablized in case things change during the stablization.

@Rustin170506
Copy link
Member

@hi-rustin, it looks like you have several open issues assigned to you. I recommend trying to finish some of those off before claiming additional issues. I appreciate the interest in helping, but claiming too many issues at the same time can make coordination more difficult.

I've claimed 4 issues, two of which have PRs waiting for review. another one is just revising the documentation, and the remaining one is also related to workspace, so I was wondering if I could look at this issue together yesterday. sorry to bother you all. I'll avoid this kind of problem next time.

@Muscraft
Copy link
Member

With workspace inheritance being stabilized, and #10112 being brought up again (#10910 would close it). I was wondering if this was something to consider again before it would be a breaking change.

Currently setting edition in a member while the workspace does not specify resolver = "2", results in using the v1 resolver being used. There is no warning for this (currently) and seems like a misstep. Having edition set in [workspace.package] imply the resolver version (if it's not set) would mitigate this issue for users of workspace inheritance. If this is something we should move forward with It would be necessary to document it.

@ehuss ehuss changed the title Infer resolver version from workspace version Infer resolver version from workspace edition May 9, 2023
@ehuss ehuss added the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label May 9, 2023
@ehuss
Copy link
Contributor Author

ehuss commented May 9, 2023

Triage: I still lean towards implementing #10112 over this for now. I think making workspace.package.edition have extra meaning could be a problem, and adding workspace.edition could be confusing. It would also be a backwards-incompatible change to change workspace.package.edition now that it is stable (at least for 2021, we could make future editions have different meaning).

It is unfortunate to need the extra boilerplate of the resolver field, but I don't really know of an alternate solution that would be better. There is likely going to be more edition-specific behavior changes in the future, so we'll likely need to figure out some solution, I just don't know what that is. Either of the above are possibilities (workspace.edition or workspace.package.edition), but I'm not particularly happy with either of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

4 participants