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

Fix ability to eliminate getopts dep #385

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Apr 24, 2021

Based on the use of [features] getopts = ["timely_communication/getopts"] it appears that it was intended for timely_communication's "getopts" feature to be only conditionally enabled. However "getopts" is a default feature of timely_communication.

In order for a crate that depends on timely to be able to avoid the getopts dependency when it has no need for timely::execute_from_args, timely must not unconditionally enable timely_communication's default features.

PR tested with a crate that depends on timely with default-features=false, using cargo check to confirm it's able to build and cargo tree to confirm the getopts transitive dependency is now correctly eliminated.

@frankmcsherry
Copy link
Member

Sorry for the hold up. This seems very appropriate!

Can you say anything about what led you here? The get_opts stuff was pulled out for the core Rust folks a while back (when Polonius used DD), because it wouldn't build as part of the bootstrap. Is this related to that? Ideally I'd just simplify everything somehow, but trying to stay abreast of the current uses.

@frankmcsherry frankmcsherry merged commit d14afd6 into TimelyDataflow:master Apr 29, 2021
@dtolnay dtolnay deleted the nogetopts branch April 29, 2021 16:56
@dtolnay
Copy link
Contributor Author

dtolnay commented Apr 29, 2021

My use case is not rustc bootstrap. I just noticed this as very clearly unintentional/oversight when reviewing my dependencies. It would not be a problem for me if getopts becomes unconditional — but as long as I don't need it and there is a way to disable it, I am going to disable it.

@frankmcsherry
Copy link
Member

Thanks for the heads up! I appreciate it. :D

@github-actions github-actions bot mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants