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

resolve the ambiguity to determine version of Cargo feature resolver. #8917

Merged

Conversation

FomalhautWeisszwerg
Copy link
Contributor

About This Pull Request

Summary

  • fix version of the Cargo feature resolver to "2".

Details

In helix, multiple edition designations are mixed as following:

$ grep --recursive --line-number "edition" helix-*
helix-core/Cargo.toml:5:edition = "2021"
helix-dap/Cargo.toml:5:edition = "2018"
helix-event/Cargo.toml:5:edition = "2021"
helix-loader/Cargo.toml:6:edition = "2021"
helix-lsp/Cargo.toml:5:edition = "2021"
helix-parsec/Cargo.toml:5:edition = "2021"
helix-term/Cargo.toml:6:edition = "2021"
helix-tui/Cargo.toml:8:edition = "2021"
helix-vcs/Cargo.toml:5:edition = "2021"
helix-view/Cargo.toml:5:edition = "2021"

And each edition implies a different default feature resolver version.

  • edition = "2018" implies resolver = "1"
  • edition = "2021" implies resolver = "2"

There is one problem. The setting of Cargo feature resolver is a global so only honored at the top-level one (and the others are ignored, the ignored resolver's features are not merged).

Currently, Cargo.toml at the top-level does not specify a feature resolver. This makes it hard for cargo to determine which is the developer's intent to use. In fact, a newer toolchain displays the following warning:

$ cargo --version
cargo 1.74.0 (ecb9851af 2023-10-18)
$ cargo build
warning: virtual workspace defaulting to `resolver = "1"` despite one or more workspace members being on edition 2021 which implies `resolver = "2"`
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
note: for more details see https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions

This pull request solve the ambiguity by specifying the feature resolver version to "2" in the root's manifest.

This commit solve the ambiguity to determin the version of resolver.
To get more detail, see the following two documents:

- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
- https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html
@dead10ck
Copy link
Member

Hmm, it's probably an oversight that helix-dap is still set to the 2018 edition. We should probably fix that too.

dead10ck
dead10ck previously approved these changes Nov 26, 2023
@pascalkuthe
Copy link
Member

I would prefer if the rust edition for helix-dap would be fixed instead of using this workaround

@Tudyx
Copy link
Contributor

Tudyx commented Nov 26, 2023

This is not a workaround, we must specify the resolver manually otherwise it gets ignored. See https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace . I got bitten by this once, i didn't understand why feature unification wasn't respected.

By the way is setting the edition in the workspace Cargo.toml and then inherite it in child Cargo.toml has been envisaged? Would look like this in the child toml:

[package]
edition.workspace = true

@pascalkuthe
Copy link
Member

I wouldn't mind. When these crates were crated workspace inheritance wasn't around yet. Either way I would still like to see the edition fixed in this PR because we will forget otherwise

Now, the Rust 2021 is available in all workspaces.
@FomalhautWeisszwerg
Copy link
Contributor Author

Thanks for reviewing and feedback.

I fixed Rust editions in ba5001b by using the workspace inheritance. Also license and rust-version.

I hope that helps.

@FomalhautWeisszwerg
Copy link
Contributor Author

FomalhautWeisszwerg commented Nov 27, 2023

@Tudyx

we still need to set the resolver explicitly in the workspace 's Cargo.toml.

This seems wrong. Because the feature resolver setting is global so it must be specified in the root's Cargo.toml only. And the feature resolver setting should be in the [workspace] (not [workspace.package] ).

If a feature resolver were specified in a non-root Cargo.toml, cargo shows warning and ignore them.
For example, resolver = "1" specified in helix-dap/Cargo.toml, cargo shows following warings:

$ cargo build
warning: resolver for the non root package will be ignored, specify resolver at the workspace root:
package:   /mnt/SHPP41-2000GM-2/rustacean/helix_editor/fix/feature_resolver/helix-dap/Cargo.toml
workspace: /mnt/SHPP41-2000GM-2/rustacean/helix_editor/fix/feature_resolver/Cargo.toml

@Tudyx
Copy link
Contributor

Tudyx commented Nov 27, 2023

Yeah by worspace's Cargo.toml, I meant the Cargo.toml at the root and only this one. Like you have done in your PR so this is fine. Sorry for the unclear terminology

@kirawi kirawi added the S-waiting-on-review Status: Awaiting review from a maintainer. label Nov 27, 2023
Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

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

Thanks!

@pascalkuthe pascalkuthe merged commit b7f98d1 into helix-editor:master Nov 27, 2023
6 checks passed
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
* fix: version of Cargo feature resolver.

This commit solve the ambiguity to determin the version of resolver.
To get more detail, see the following two documents:

- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
- https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html

* unified: Rust edition in all workspaces.

Now, the Rust 2021 is available in all workspaces.

* fined up: Cargo.toml by using workspace inheritance.

To get more detail of the `workspace.package` table, see a following document:

- https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
* fix: version of Cargo feature resolver.

This commit solve the ambiguity to determin the version of resolver.
To get more detail, see the following two documents:

- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
- https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html

* unified: Rust edition in all workspaces.

Now, the Rust 2021 is available in all workspaces.

* fined up: Cargo.toml by using workspace inheritance.

To get more detail of the `workspace.package` table, see a following document:

- https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
* fix: version of Cargo feature resolver.

This commit solve the ambiguity to determin the version of resolver.
To get more detail, see the following two documents:

- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
- https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html

* unified: Rust edition in all workspaces.

Now, the Rust 2021 is available in all workspaces.

* fined up: Cargo.toml by using workspace inheritance.

To get more detail of the `workspace.package` table, see a following document:

- https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* fix: version of Cargo feature resolver.

This commit solve the ambiguity to determin the version of resolver.
To get more detail, see the following two documents:

- https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions
- https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html

* unified: Rust edition in all workspaces.

Now, the Rust 2021 is available in all workspaces.

* fined up: Cargo.toml by using workspace inheritance.

To get more detail of the `workspace.package` table, see a following document:

- https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants