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

Tracking issue for namespaced-features #5565

Closed
1 task done
djc opened this issue May 24, 2018 · 57 comments · Fixed by #10269
Closed
1 task done

Tracking issue for namespaced-features #5565

djc opened this issue May 24, 2018 · 57 comments · Fixed by #10269
Labels
A-namespaced-features Area: namespaced-features C-tracking-issue Category: A tracking issue for something unstable.

Comments

@djc
Copy link
Contributor

djc commented May 24, 2018

Docs: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#namespaced-features
Original issue: #1286
Implementation: #5300

Not sure what the process is for stabilizing experimental Cargo features, I figured a tracking issue might be useful? @alexcrichton any guide on what should be done to move this towards stabilization?

Unresolved issues

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

@alexcrichton
Copy link
Member

Indeed a tracking issue is useful! I think though this still has a large piece to implement which is to propagate namespaced features to the registry index

@djc
Copy link
Contributor Author

djc commented May 24, 2018

Yes, I figured as much. So do you have any ideas/guidance about how to go about that? Are there established ideas on how to evolve the registry index?

@alexcrichton
Copy link
Member

The evolution here is just a new key in the registry which we've done a bunch of times before, so it's fine to do whenever once there's an agreed up on design. Most of the time this is hard to undo though so we want to be pretty sure of the design before it lands. Technically it'll involve modifying crates.io as well as the publishing code in Cargo to send more data to the registry

@djc
Copy link
Contributor Author

djc commented May 24, 2018

Can you be a bit more specific about which parts of the cargo and/or crates.io code need changing?

@alexcrichton
Copy link
Member

You may want to hop onto the crates.io channel on discord as they may know more, but it's all around krate.rs and git.rs IIRC

@matklad matklad added the C-tracking-issue Category: A tracking issue for something unstable. label Jun 26, 2018
@djc
Copy link
Contributor Author

djc commented Jun 27, 2018

@alexcrichton okay, so, I'm still not entirely clear on what needs to be done on the crates.io side here. Is it just propagating the package.namespaced-features value? The feature values can still be serialized as a string, so that's the least invasive change. Alternatively, we could add more structure to the feature values and use serde's enum handling to have a new key that's more explicit about what's what.

Either way, I'm not sure how the compatibility with old cargo is supposed to work, or who all needs to be on board with a supposed design.

@alexcrichton
Copy link
Member

@djc there's a few things here I think that we'd need to consider:

  • One aspect is that we need to consider what happens when older versions of Cargo are used. For example if I use Cargo from 1.20.0 with a crate that has a namespaced feature, what happens? Is an error generated? Does the package not exist? Does it silently work sometimes and not others? My preferred way to solve this would be to add support for this today but don't actually allow publishing crates just yet. That way after a few cycles and Cargo rides the trains older Cargo will be able to understand newer crates in the future.
  • Next is the actual registry format itself. Right now the features in the registry index are just an array of strings, but presumably yeah we'll need to propagate the namespaced features boolean as well. New registry fields could be used as well.

Before we go much further though I think it'd be good to get weigh-in from @rust-lang/cargo because once we start changing the registry it's sort of the point of no return

@joshtriplett
Copy link
Member

@alexcrichton So, this is exactly the kind of change that really motivated the idea of supporting Cargo version dependencies in a way that older versions of Cargo can handle.

We can't make old versions of Cargo unable to parse new versions of the index; that's a serious problem.

If we're going to do this, we need to version the index, such that older versions of Cargo don't see crates that use this feature at all.

@alexcrichton
Copy link
Member

@joshtriplett indeed! To be clear though the main issue is that an older Cargo won't understand the newer index, but we get to control how it doesn't understand the new index as well (aka no features, a mapping of features, or something like that). Having a fully versioned index seems far off still so I'd be fine implementing this before that comes along.

Now having a versioned index is a good idea though and we can start at any time to add safeguards into Cargo to avoid looking at future revisions!

@alexreg
Copy link
Contributor

alexreg commented Aug 10, 2018

Is this going to move towards stabilisation soon?

@djc
Copy link
Contributor Author

djc commented Aug 10, 2018

Probably not -- I'm still motivated to write code to move this forward, but I don't know exactly what needs to be done.

@alexreg
Copy link
Contributor

alexreg commented Aug 10, 2018

Oh, I thought it was already implemented, just needed stabilising.

@djc
Copy link
Contributor Author

djc commented Aug 10, 2018

The Cargo part is implemented -- it apparently needs complementary stuff in crates.io.

@alexreg
Copy link
Contributor

alexreg commented Aug 10, 2018

Aha.

What I still don't understand is how this issue relates to implicit features for dependencies, exactly. (I was linked here from that issue.)

@ExpHP
Copy link
Contributor

ExpHP commented Aug 11, 2018

What I still don't understand is how this issue relates to implicit features for dependencies, exactly. (I was linked here from that issue.)

I agree that more could be said here. I find it odd how none of the examples I can locate for this feature show how it can be used to overcome the limitations of optional dependencies (which were its primary motivation)!

See this page:

And now let me try to put it in context:


Suppose you have this:

[dependencies]
rand = { version = "0.5.5", optional = true }

Without namespaced-features, an optional dep implicitly defines a feature that cannot be overridden:

[features]
rand = [] # implicitly generated, always

...and rand is added to the dependency tree whenever the feature rand is required.

With namespaced-features, an optional dep implicitly defines a feature that can be overridden:

[features]
rand = ["crate:rand"]  # implicitly generated, unless you write your own rand feature

...and rand is added to the dependency tree whenever the feature crate:rand is required.


If you look closely, the actual namespacing of the dependencies does not, in itself, have anything to do with the issue of implicit features. It is the other changes enabled by the namespaced-features feature (notably the ability to override the implicit feature) that make a difference.

The justification for tying these changes together was given in #1286. Er... maybe. Actually, I can't really find it explicitly spelled out there. But the proposal is strictly more powerful than if we were to simply allow the implicit feature to be overridden with no other changes (because this also allows you to have a rand feature that does not toggle the rand dependency. Why? Idunno.).

@alexreg
Copy link
Contributor

alexreg commented Aug 12, 2018

@ExpHP Thanks, that explanation makes a lot more sense to me. Was there a reason @sfackler's original proposal wasn't chosen though? I kind of like the idea of doing something like

[features]
a = { features = ["b", "c/d"], dependencies = ["c"] }

or such.

@tikue
Copy link

tikue commented Oct 16, 2018

Is it possible to publish a crate that uses this feature? I'm getting:

error: api errors: invalid upload request: invalid value: string "crate:serde", expected a valid feature name at line 1 column 1862

@alexcrichton
Copy link
Member

@tikue I believe not currently. In addition to crates.io checks the registry index isn't ready for namespaced features

@tikue
Copy link

tikue commented Oct 17, 2018

@alexcrichton thanks, it was easy enough to work around, just had to name a serde feature serde1.

@alexreg
Copy link
Contributor

alexreg commented Oct 17, 2018

@alexcrichton Does an issue for that exist on the crates.io repo?

@alexcrichton
Copy link
Member

@alexreg I think that's this issue, the tracking issue for this feature. The feature needs to be designed for crates.io first.

@alexreg
Copy link
Contributor

alexreg commented Oct 17, 2018

@alexcrichton Hmm? That's what I mean, we need to implement namespaced features on crates.io. There should be a separate issue for that over on that repo, I think, no?

@djc
Copy link
Contributor Author

djc commented Oct 18, 2018

FWIW, I'd like to move this forward, but I'm currently having a hard time figuring out exactly what needs to be done about the index and crates.io. If someone can provide me with more guidance on that, that would be great.

@alexcrichton
Copy link
Member

@djc I think that rust-lang/crates.io#1487 may be helpful in providing a rough template for how to make progress?

@est31
Copy link
Member

est31 commented Oct 25, 2020

Regarding the crate:foo syntax, I think it'll confuse a lot of people that crate:foo in Cargo.toml refers to the foo external crate (in fact, package), while crate::foo in mod.rs refers to the crate's internal module named foo.

Maybe it would be better to use pkg:foo instead of crate:foo?

bors added a commit that referenced this issue Oct 27, 2020
New namespaced features implementation.

This is a new implementation for namespaced features (#5565). See the `unstable.md` docs for a description of the new behavior. This is intended to fix several issues with the existing design, and to make it backwards compatible so that it does not require an opt-in flag.

This also includes tangentially-related changes to the new feature resolver. The changes are:

* `crate_name/feat_name` syntax will now always enable the feature `crate_name`, even if it is an inactive optional dependency (such as a dependency for another platform). The intent here is to have a certain amount of consistency whereby "features" are always activated, but individual crates will still not be activated.
* `--all-features` will now enable features for inactive optional dependencies. This is more consistent with `--features foo` enabling the `foo` feature, even when the `foo` dep is not activated.

I'm still very conflicted on how that should work, but I think it is better from a simplicity/consistency perspective. I still think it may be confusing if someone has a `cfg(some_dep)` in their code, and `some_dep` isn't built, it will error. The intent is that `cfg(accessible(some_dep))` will be the preferred method in the future, or using platform `cfg` expression like `cfg(windows)` to match whatever is in Cargo.toml.

Closes #8044
Closes #8046
Closes #8047
Closes #8316

## Questions
- For various reasons, I changed the way dependency conflict errors are collected. One slightly negative consequence is that it will raise an error for the first problem it detects (like a "missing feature"). Previously it would collect a list of all missing features and display all of them in the error message. Let me know if I should retain the old behavior. I think it will make the code more complicated and brittle, but it shouldn't be too hard (essentially `Requirements` will need to collect a list of errors, and then `resolve_features` would need to check if the list is non-empty, and then aggregate the errors).

- Should `cargo metadata` show the implicit features in the "features" table? Currently it does not, and I think that is probably best (it mirrors what is in `Cargo.toml`), but I could potentially see an argument to show how cargo sees the implicit features.
@ehuss
Copy link
Contributor

ehuss commented Jan 13, 2021

As discovered in #8832, this unfortunately breaks Cargo versions older than 1.19, even if they have a Cargo.lock file. We'll need to figure out how to address that concern before this can move forward.

@ehuss
Copy link
Contributor

ehuss commented Apr 5, 2021

I forgot to post a followup here. #9161 implemented changes to support namespaced features in the index and avoid breaking older versions of cargo. There is some risk that versions older than 1.51 may resolve packages using namespaced features in such a way that those features don't get activated. I'll feel a little more comfortable once more time has passed so that 1.51 is at least a few months old.

@ehuss
Copy link
Contributor

ehuss commented Jun 22, 2021

I have posted a proposal to stabilize this in rust-lang/rfcs#3143.

bors bot added a commit to georust/gpx that referenced this issue Oct 3, 2021
62: Implementing serde (De)Serialize for GPX structs r=lnicola a=mkroehnert

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGELOG.md` if knowledge of this change could be valuable to users.
---

This PR resolves #59, by implementing optional support for (de-)serializing the GPX structs via serde.

I named the feature flag `serde-serialize`, same as in the `wasm-bindgen` crate.
Using `serde` for the feature flag name is not yet possible, since it would require the currently unstable [cargo namespaced features](https://doc.rust-lang.org/cargo/reference/unstable.html#namespaced-features) implementation.
See also the related [cargo tracking issue](rust-lang/cargo#5565).

It was asked in #59, whether it would be possible to use GeoJSON, but for my usecase, it would be overkill to deal with additional data structure conversions.
I also checked the [Rust API guidelines](https://rust-lang.github.io/api-guidelines/interoperability.html#c-serde), which mention that it would be good, if structs implemented `serde::{Serialize, Deserialize}`.

If the changes are okay and can/should be merged, I'll update the branch with a changelog entry.

Co-authored-by: Manfred Kroehnert <7868+mkroehnert@users.noreply.github.com>
@taralx
Copy link

taralx commented Oct 6, 2021

Proposal passed, what's next? If there are work items people could pick up, could they be added to the initial comment?

@taralx
Copy link

taralx commented Oct 6, 2021

Also, out of curiosity, why doesn't #[cfg(feature="dep:bar")] work?

@ehuss
Copy link
Contributor

ehuss commented Oct 6, 2021

Proposal passed, what's next?

This is just waiting for a review on a PR on crates.io to accept the new syntax.

Also, out of curiosity, why doesn't #[cfg(feature="dep:bar")] work?

I'm not sure I fully understand your question, since the point of the dep: syntax is to avoid setting features, and dep:bar would be invalid syntax for a cfg expression. The intent is that cfg(accessible(...)) will be the intended way in the future for detecting the presence of a dependency, which is discussed in the rfc.

@ehuss
Copy link
Contributor

ehuss commented Jan 6, 2022

Posted #10269 as a proposal to stabilized this.

@runiq
Copy link

runiq commented Feb 2, 2022

Huzzah, #10269 has been merged! :)

@ehuss
Copy link
Contributor

ehuss commented Feb 2, 2022

Oops, looks like I closed the wrong issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-namespaced-features Area: namespaced-features C-tracking-issue Category: A tracking issue for something unstable.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.