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

RFC: Cargo feature visibility (private/public) #3487

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Sep 9, 2023

Rendered

RFC for feature-visibility
RFC goals: add a way to make Cargo features private

This was split from #3416

[features]
foo = { enables = [], public = false}

This would resolve rust-lang/cargo#10882

@compiler-errors compiler-errors added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Sep 9, 2023
These are meant to apply simple feedback that wouldn't change the
author's intent.
Comment on lines 92 to 95
This feature requires adjustments to the index for full support. This RFC
proposes that it would be acceptable for the first implementation to simply
strip private features from the manifest; this meanss that there will be no way
to `cfg` based on these features.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we strip them, that affects a couple use cases

  • When helping with managing the feature graph, you might want to cfg based on this
  • Activating debug features in clap or winnow via --features winnow/debug

Copy link
Contributor Author

@tgross35 tgross35 Sep 15, 2023

Choose a reason for hiding this comment

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

In that case, do you think it is worth including the index change as part of the MVP?

Copy link
Contributor

Choose a reason for hiding this comment

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

The index change can be fairly minor

  • Add a new private-features = [<name>, ...] entry
    • private because those should be in the minority and keeps the index smaller
  • Only allow them to be used when MSRV is high enough
    • Question is what to do when MSRV is unset.
    • It might be of interest for us to add other MSRV fields, like published-rust-version to record what version of rust was cargo publish done in and a calculated-rust-version that isn't as precise as rust-version but is a minimum value based on things like setting the edition or using private features
    • Alternatively, we could just make it a warning: "Hey, your rust-version is unset, if you publish with private features, older versions of cargo can use them and then will. break on upgrade"

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I can foresee people requesting for workspace public/private features. How does this RFC interact with Cargo workspaces?

text/3487-feature-visibility.md Outdated Show resolved Hide resolved
This RFC describes a new key under `features` in `Cargo.toml` to indicate that a
feature is private.

Please see the parent meta RFC for background information: [`feature-metadata`].
Copy link
Member

Choose a reason for hiding this comment

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

the link is broken

Copy link
Contributor Author

@tgross35 tgross35 Sep 15, 2023

Choose a reason for hiding this comment

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

Thanks, a lot of links got reshuffled when I split the RFCs. I will fix this

This feature requires adjustments to the index for full support. This RFC
proposes that it would be acceptable for the first implementation to simply
strip private features from the manifest; this means that there will be no way
to `cfg` based on these features.
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what cfg means here. I assumed it is the cfg attribute and the cfg macro. Could you expand and link to their doc respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of forgot the exact context here, this comes from Ed's comment at #3416 (comment) and the two following comments. Also #3416 (comment).

Do you have any ideas for how to improve the wording?

Thank you for taking a look by the way

Copy link
Contributor

Choose a reason for hiding this comment

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

If we strip the private features then when cargo does a build, it won't tell rustc about private features and if you do #[cfg(feature = "private-foo")] then it will never evaluate to true

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
@tgross35
Copy link
Contributor Author

I can foresee people requesting for workspace public/private features. How does this RFC interact with Cargo workspaces?

Do you mean crates within a workspace wanting to use private features for other crates in the workspace? If so, I previously had a --allow-private-features flag as part of this proposal that would cover that, but I removed it recently. Do you have any suggestions for how we could handle this?

@ijackson
Copy link

ijackson commented Oct 3, 2023

Do you mean crates within a workspace wanting to use private features for other crates in the workspace? If so, I previously had a --allow-private-features flag as part of this proposal that would cover that, but I removed it recently. Do you have any suggestions for how we could handle this?

How about public = "workspace"

Comment on lines +95 to +98
This feature requires adjustments to the index for full support. This RFC
proposes that it would be acceptable for the first implementation to simply
strip private features from the manifest; this means that there will be no way
to `cfg` based on these features.
Copy link

Choose a reason for hiding this comment

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

I think stripping features out on publish is a bad idea. I haven't thought through all the ramifications, but having different behaviour for the crates.io tarball and for the actual source code from git seems like a beartrap.

I'm not sure why any changes are needed to the infrastructure for initial support. The infrastructure would display the features and convey them to clients, and it would be up to clients to enforce them.

However, we do need something that a user of this feature can put in their crate that will prevent old versions of cargo from failing to honour the public flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this is already being discussed some in #3487 (comment)

As for why infrastructure is needed is a "it depends". There are a lot of ways to slice and dice things to possibly make things work.

When dependency resolution happens, it works off of a Summary of the Cargo.toml that is stored inside the Index. If we want to prevent use when resolving dependencies, it must be present in the Summary. We could instead do a second pass on the dependency tree, checking for use of private features. This would likely break down quickly as we don't know why features are enabled (other features from within the package or the caller). Setting that aside, this would require downloading and extracting of .crate files which we normally do lazily. So we either lose the lazy approach or we only do the verification for what is currently being built which means the answer won't always be visible but you need to test every platform and feature combination.

@epage
Copy link
Contributor

epage commented Oct 3, 2023

Do you mean crates within a workspace wanting to use private features for other crates in the workspace? If so, I previously had a --allow-private-features flag as part of this proposal that would cover that, but I removed it recently. Do you have any suggestions for how we could handle this?

How about public = "workspace"

A workspace level feature can work fine locally but fails on publish as we have no concept of workspaces in the registry. That is a whole system to design that seems out of scope of this RFC and at most should be left to a future possibility.

Comment on lines +79 to +80
- Referenced in `[[bench]]` and `[[test]]` target `required-features`
- Using the feature on the command-line is allowed
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expand this to all required-features? That way we can do things like have private bins for testing purposes

Comment on lines +91 to +93
Attempting to use a private feature in any of the forbidden cases should result
in an error. Exact details of how features work will likely be refined during
implementation and experimentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

  • cargo install? Is that more like a [dependencies] entry and forbidden or like cargo check --features and allowed?
    • Does the answer change if we are doing the install from registery vs git vs path?
  • What about path dependencies? Should we allow workspace members to have access to private details?
    • If not, what about a path = "." dependency? These are used in dev-dependencies to enable features specific to testing

The answer to these isn't necessarily "yes" or "no" but can also be "not yet, we'll error for now and re-evaluate in the future" at which point it should be in the future possibilities.

@@ -0,0 +1,165 @@
- Feature Name: feature-metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

feature name should be updated

@epage epage added the S-waiting-on-author Status: This is awaiting some action from the author. label May 21, 2024
This RFC describes a new key under `features` in `Cargo.toml` to indicate that a
feature is private.

Please see the parent meta RFC for background information: [`feature-metadata`].
Copy link
Contributor

Choose a reason for hiding this comment

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

This RFC is blocked on feature-metadata

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
Status: RFC needs review
Status: Unreviewed
Development

Successfully merging this pull request may close these issues.

private/hidden features
6 participants