Target features #1197

[plyhun]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks @plyhun! This is pretty light on details though, mind filling in some information? Currently this has two commits, one of which says "Target features" and the other which says "Rustfmt". There's a long string of PRs I think leading to this one, but it'd be great to fill out a description and/or commit messages explaining what this PR does and how it does it.

[plyhun]

Thanks for the quick response @alexcrichton .

In principle, my changes are:

• The str/String/InternedString that represents a feature, is replaced with a tuple (str/String/InternedString, Option<Platform>) , and Set<String> is replaced with Map<String, Option<Platform>>, for the places where the parser interacts with the input Cargo.toml and the platform triple. This gives every feature a mandatory companion of Option<Platform>.
• The Platform itself has been moved from core::dependency to cfg::util to generalize its purpose.
• The Cargo.toml parser has been upgraded to support the feature target.
• The core check is upgraded for general purpose.
• ... and its various usages: optional deps check, docs compile, code compile
• As we resolve the platform at the compile stage, the platform should be carried over all the feature resolutions
• Test cases that depict new possibilities
• Metadata test updated to support the new platform field

Thus, the feature is still distinguished by a name, as before. Which is disputable, as only allows the unique feature being enabled or disabled according to the target cfg. For instance:

# does not make sense on unix
[target.'cfg(windows)'.features]
winapi = []
# does not make sense on windows
[target.'cfg(unix)'.features]
root-utils = []

Perhaps a move worth trying is to merge a feature name with platform, making them a single feature key, so the following constructions are also allowed:

// This is not implemented!
[target.'cfg(windows)'.features]
dep = ["dep2/windows"]
[target.'cfg(unix)'.features]
dep = ["dep2/unix"]

I did not dare to refactor the code that tough yet.

[Eh2406]

Sorry for the long delay in replying, I did not have internet access for the past week. This is passing because it uses both c v1.0.0 and c v1.1.0 witch is only allowed as they are path dependencies. What happens when using Package::new("c", "1.0.0").publish() to test as Crates.io dependencies?

(btw you can rebase or merge in the same branch instead of opening new prs)

[plyhun]

No worries. It may wait.
Have struggled fixing the windows' crlfs in place, so eventually had to recreate a PR.

[Eh2406]

I am missing something, how is this a backwards compatible change to the metadata format?

[plyhun]

Bah. My fault - I remember explicitly implementing this compatibility. The commit is on the way.

[joshtriplett]

We discussed this in the cargo team meeting, and we agreed that this isn't something that should just directly appear as a PR on the Cargo repository. We agreed that this needs an RFC clearly discussing the user-visible behavior, use cases, and similar.

If such an RFC gets accepted, this PR could be updated or a new one opened.

[plyhun]

Thanks for the update. That's fair. Will prepare the RFC asap.

The last question, according to the comment above (last code block) - will it worth to change the implementation over (String, Platform) keys rather leaving just string keys? So there will be a possibility to have the same feature behaving differently on different platforms.

[bors]

☔ The latest upstream changes (presumably #7251) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

I'm gonna close this since it's been inactive for some time now and we decided previously that an RFC will be needed for this.