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

Features 2.0 meta tracking issue. #8088

Closed
ehuss opened this issue Apr 8, 2020 · 8 comments · Fixed by #8997
Closed

Features 2.0 meta tracking issue. #8088

ehuss opened this issue Apr 8, 2020 · 8 comments · Fixed by #8997
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-tracking-issue Category: A tracking issue for something unstable.

Comments

@ehuss
Copy link
Contributor

ehuss commented Apr 8, 2020

This is a tracking issue for a collection of changes to features that we are considering rolling out together.

The current idea is to add a flag to [workspace] to opt-in to the new behavior for all of these at once.

@ehuss ehuss added C-tracking-issue Category: A tracking issue for something unstable. A-features2 Area: issues specifically related to the v2 feature resolver labels Apr 8, 2020
@alexcrichton
Copy link
Member

For rolling out, I think I might actually propose a scheme such as:

  • Add a package.cargo-features = "2020" option to the manifest.
  • The default, if not specified, is "2015" (the 1.0 date)
  • The "2020" value indicates "interpret cargo features according to 2020 rules", same for 2015 (basically 1.0 vs 2.0)
  • When a future edition = "2021" comes around, that implies cargo-features = "2020" or whatever is latest by that point.
  • We'll update cargo new to generate cargo-features = "2020" in all new packages
  • The selected interpretation of cargo features is the maximum of all workspace members.

I think that should satisfy all our constraints, except for maybe being a nice system. The reason I propose this is the fourth bullet, though, where eventually we do not want to require this configuration and instead we will simply want to infer it from an edition = "2021" flag. If we take that as a given, [workspace] doesn't currently have an edition flag and it'd be kind a bummer if we had to generate:

[package]
edition = "2021"

[workspace]
edition = "2021"

in the next edition. So given that constraint that we want to infer it from edition = "2021" eventually, then we need some sort of way of rationalizing what happens when you configure it in [package], which taking the maximum I think is reasonable-ish.

@ehuss
Copy link
Contributor Author

ehuss commented Apr 9, 2020

I might be reluctant to use cargo-features, to avoid confusion with the top-level cargo-features which is used for nightly unstable stuff.

Hm, what about just allowing some cargo-features on stable? Like:

cargo-features = ["features2"]
[package]
name = "foo"
#

Or we could rename that, since the word "feature" is so overloaded.

Top-level things don't work as well in a virtual manifest if we want to make it part of the next edition. Unless we move the edition field to the root.

I think with looking at every workspace member, it could be too easy to accidentally introduce a global change. Imagine a large workspace with lots of different people working on it, someone introduces a new tool or library with cargo new, and doesn't know that the flag will affect everyone in the workspace.

@alexcrichton
Copy link
Member

I would slightly prefer in terms of bikeshedding that this is placed in [package] instead of the top-level since it feels pretty weird going at the top level (very little else does on stable), but that's a minor point.

I agree that changing any package with this flag (or using cargo new to create a new one) is weird to have the subtle effect of having a global change in the workspace. That being said though I don't know how we can get away from that. If we're 5 years down the road in the new edition and you cargo new into a workspace you wrote today, what's supposed to happen? I guess I sort of see it as a sort of inevitability that we're going to need to accept this subtle changes-the-global-behavior behavior, or if not I think we should focus temporarily on where we want to be after the next edition and work backwards from there to how the opt-in should look today. (if that makes sense)

I'm personally relatively wary of trying to make cargo new super smart about workspaces and doing lots of inference about how to insert itself into a workspace (or "I would be in this workspace so let me act on that"). I do think though that cargo new should always default you to the "latest and greatest defaults", which ideally is one line in Cargo.toml in the limit of time with editions. I guess put another way I don't personally see a feasible way forward to upgrade the workspace in cargo new to give us an alternative way to implement this in the future.

bors added a commit that referenced this issue Apr 20, 2020
Add `resolver` opt-in for new feature resolver.

This adds a new `resolver` field to `Cargo.toml` to provide an opt-in mechanism for backwards-incompatible resolver changes. This enables the new feature resolver and `-Zpackage-features`. See `unstable.md` for documentation.

cc #8088
@ehuss
Copy link
Contributor Author

ehuss commented Apr 24, 2020

The new opt-in mechanism is now on nightly:

cargo-features = ["resolver"]

[package]
name = "my-package"
version = "1.0.0"
resolver = "2"

See the docs for more details: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#resolver

@Amanieu
Copy link
Member

Amanieu commented Apr 25, 2020

So I just tried this out in one of my projects where I have a virtual workspace at the root and a bunch of crates in subdirectories. If I understand correctly, I only need to set the resolver in the workspace, not in the individual crates. However when I do that, I get a bunch of warnings from cargo:

warning: resolver for the non root package will be ignored, specify resolver at the workspace root:
[[repeats for each crate in the workspace]]

Is this the expected behavior? Do I need to specifically override the resolver for all crates in the workspace?

@ehuss
Copy link
Contributor Author

ehuss commented Apr 25, 2020

Nah, it looks like the warning detection is just horribly broken. It is intended that you only specify it in the root [workspace]. I'll try to get it fixed soon.

@cbiffle
Copy link
Contributor

cbiffle commented May 4, 2020

I left some glowing praise for the resolver 2 behavior in a comment on the host_dep bug, btw. Saved our bacon. Behaves well at nightly-2020-05-01.

@berkowski
Copy link

Seems to work for me as well in a use case similar to @cbiffle when using bindgen as a build dependency for a firmware project cross-compiled to thumbv6m. Not willing to make the move to nightly at this point though.

bors added a commit that referenced this issue Jul 9, 2020
…hton

Allow configuring unstable flags via config file

# Summary

This fixes #8127 by mapping the `unstable` key in `.cargo/config` to Z flags.

It should have no impact on stable/beta cargo, and on nightlies it gives folks the ability to configure Z flags for an entire project or workspace. This is meant to make it easier to try things like the [new features resolver](#8088) behavior, mtime-on-use, build-std, or timing reports across a whole project.

I've also included a small (but entirely independent) ergonomics change -- treating dashes and underscores identically in Z flags. That's along for the ride in this PR as the last commit, and is included here because it makes for more idiomatic toml file keys (`print_im_a_teapot = yes` vs `print-im-a-teapot = yes`). Please let me know if y'all would prefer that be in a separate PR, or not happen at all.

# Test Plan

Apologies if I've missed anything -- this is my first cargo contrib and I've tried to hew to the contributing guide. If I've slipped up, please let me know how and I'll fix it.

NB. My linux machine doesn't have multilib set up, so I disabled cross tests.

 * `cargo test` passes for each commit in the stack
 * I formatted each commit in the stack with `rustfmt`
 * New tests are included alongside the relevant change for each change
 * I've validated each test by locally undoing the code change they support and confirming failure.
 * The CLI wins, for both enable and disabling Z flags, as you'd expect.

Keys in `unstable` which do not correspond with a Z flag will trigger an error indicating the invalid flag came from a config file read:

```
Invalid [unstable] entry in Cargo config

Caused by:
  unknown `-Z` flag specified: an-invalid-flag
```

If you'd like to see a test case which isn't represented here, I'm happy to add it. Just let me know.

# Documentation

I've included commits in this stack updating the only docs page that seemed relevant to me, skimming the book -- `unstable.md`.
@bors bors closed this as completed in 4aa5223 Jan 5, 2021
@bors bors closed this as completed in #8997 Jan 5, 2021
FlorianRohm added a commit to TNG/gs-rs that referenced this issue Apr 29, 2021
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 C-tracking-issue Category: A tracking issue for something unstable.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants