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

Remove dependency on proc-macro-hack #2520

Merged
merged 3 commits into from Nov 23, 2021
Merged

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Nov 23, 2021

This also removes dependency on autocfg & proc-macro-nested.

This bumps MSRV of utility crates (futures, futures-util, futures-executor, etc.) to 1.45.
The Rust 1.45 release is over a year old , Debian stable's rustc version is 1.48, Ubuntu's rustc version is 1.53, so this meets our tentative MSRV policy on utility crates.

@taiki-e taiki-e added 0.3-backport: pending A-macro labels Nov 23, 2021
@taiki-e taiki-e merged commit 9f73b8d into master Nov 23, 2021
22 checks passed
@taiki-e taiki-e deleted the taiki-e/proc-macro-hack branch Nov 23, 2021
@taiki-e taiki-e mentioned this pull request Nov 23, 2021
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending labels Nov 23, 2021
@taiki-e taiki-e mentioned this pull request Nov 23, 2021
@carols10cents
Copy link
Member

carols10cents commented Nov 29, 2021

Just FYI, this PR was backported and released in 0.3.18, and it removed the features proc-macro-hack and proc-macro-nested that were part of the default feature set of futures-util.

The Cargo docs recommend not removing features in semver-compatible releases:

The resolver will skip over versions of packages that are missing required features. For example, if a package depends on version ^1 of regex with the perf feature, then the oldest version it can select is 1.3.0, because versions prior to that did not contain the perf feature. Similarly, if a feature is removed from a new release, then packages that require that feature will be stuck on the older releases that contain that feature. It is discouraged to remove features in a SemVer-compatible release. Beware that optional dependencies also define an implicit feature, so removing an optional dependency or making it non-optional can cause problems, see removing an optional dependency.

More documentation here.

I've fixed the problem in the project I'm working on, but it took a while to figure out this was the root cause preventing cargo update from updating the project from futures-util 0.3.17 to 0.3.18.

Thanks!

@taiki-e
Copy link
Member Author

taiki-e commented Nov 29, 2021

@carols10cents

proc-macro-hack (and proc-macro-nested) was an optional dependency, and a feature named "proc-macro-hack" was not used anywhere in our crate (i.e., and would be completely useless even if it were specified), and was not documented anywhere. -- So, it is not considered a public API.

I don't know why your project specified such an optional dependency as a feature, but I would recommend that you don't specify as a feature an optional dependency that is undocumented and unused as a feature.

Or please open an issue and ask for clarification before specifying them as features -- In the case of futures, all of the undocumented optional dependencies are not considered features.

@carols10cents
Copy link
Member

carols10cents commented Nov 29, 2021

We specified it as part of a workspace-hack crate (like rustc does) using cargo-hakari, which unifies all features as Cargo does to minimize the number of crate rebuilds.

Optional dependencies automatically create feature names, and Cargo doesn't provide a way to designate features as "public" or "private" unfortunately.

Again, I just wanted you to know about Cargo best practices for the future.

@taiki-e
Copy link
Member Author

taiki-e commented Jan 2, 2022

We specified it as part of a workspace-hack crate (like rustc does) using cargo-hakari, which unifies all features as Cargo does to minimize the number of crate rebuilds.

Optional dependencies automatically create feature names, and Cargo doesn't provide a way to designate features as "public" or "private" unfortunately.

Given that it is incompatible with half of cargo's recommended mitigation strategies, running cargo update in the usual way in a workspace that uses cargo-hakari seems to be a problem in itself.

  • Clearly document your features. If the optional dependency is not included in the documented list of features, then you may decide to consider it safe to change undocumented entries.
  • Use high-level features which enable optional dependencies, and document those as the preferred way to enable the extended functionality. For example, if your library has optional support for something like "networking", create a generic feature name "networking" that enables the optional dependencies necessary to implement "networking". Then document the "networking" feature.

If I understand correctly, I think the better solution here is not to ask the library maintainers to handle this issue, but to implement a subcommand like cargo hakari update that temporarily disables workspace-hack, runs cargo update, and then re-enables workspace-hack with updated deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants