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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce scope of tokio dependencies #10

Merged
merged 6 commits into from Dec 26, 2018

Conversation

Projects
None yet
2 participants
@lopopolo
Copy link
Contributor

lopopolo commented Dec 26, 2018

Hi. Thank you for this library 馃槃

tokio is a fat dependency that brings in a bunch of packages that are not needed by futures-locks: things like tokio-codec and tokio-fs.

The tokio docs suggest that the tokio crate should only be depended on by application crates. It suggests that libraries should only depend on the subcrates they need.

I've taken a first stab at reducing the tokio dependencies to their minimum set (tokio-current-thread, tokio-executor). Due to a cargo bug, I couldn't keep the feature name the same to make this a transparent change.

I expect that this patch isn't mergeable as is, but am happy to iterate until we get it right 馃洜

Happy new year 馃帀

lopopolo added some commits Dec 26, 2018

@asomers

This comment has been minimized.

Copy link
Owner

asomers commented Dec 26, 2018

I don't understand the Cargo bug. Why do you need to change the feature's name?

@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

The feature name tokio clashes with the dev-dependency of the same name because the two targets share a feature namespace.

@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

I suppose we could work around this by depending on tokio_executor and tokio_runtime directly in tests.

@asomers

This comment has been minimized.

Copy link
Owner

asomers commented Dec 26, 2018

So why not just change the dev dependency's name? That way there wouldn't be any impact on users.

@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

So why not just change the dev dependency's name?

馃憤 I didn't know that was possible! I aliased tokio to tokio_ in tests and aliased back to tokio with the extern crate statement. Let me know if this needs tweaking 馃槃

@asomers

This comment has been minimized.

Copy link
Owner

asomers commented Dec 26, 2018

Looks like Rust 1.26.0 doesn't support renaming crates. I'm not averse to raising the minimum rust version, though, if it's for a good reason. Could you please find out which is the lowest version of Rust that supports this feature?

@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

The rename-dependencies feature was recently stabilized in cargo 1.31.0: PR, merge commit, docs,

The feature was first introduced in cargo 1.27.0, nightly only, behind a flag.

@asomers

This comment has been minimized.

Copy link
Owner

asomers commented Dec 26, 2018

Well, I guess we should make 1.31.0 the minimum then. I don't see any a better alternative.

@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

馃憤

@asomers

This comment has been minimized.

Copy link
Owner

asomers commented Dec 26, 2018

Also, there's two other things you must do:

  1. Reference the cargo bug in a comment inside Cargo.toml. Naming the feature tokio_ is pretty non-obvious.
  2. Ensure that the Travis builds run with the tokio_ feature enabled.
  3. Add a CHANGELOG entry noting that the minimum compiler version has increased.
@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

  1. the tokio_ dep is always included in test builds since it is a dev dependency. The tokio feature is enabled in tests by default since it is a default feature.

Thanks for helping me pull this together 馃槃

@asomers
Copy link
Owner

asomers left a comment

Ok, it all looks good to me! Thanks for your contribution.

@asomers asomers merged commit 2764634 into asomers:master Dec 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lopopolo

This comment has been minimized.

Copy link
Contributor

lopopolo commented Dec 26, 2018

馃帀 馃帀 馃帀

asomers added a commit that referenced this pull request Dec 26, 2018

Raise tokio dev dependency to 0.1.8
This was an oversight from PR #10.  The tests don't compile with tokio
less than 0.1.7, and they don't pass with tokio less than 0.1.8.  CI
didn't reveal the problem because new repos use the latest crates by
default.

asomers added a commit that referenced this pull request Dec 26, 2018

Raise tokio dev dependency to 0.1.8 (#11)
This was an oversight from PR #10.  The tests don't compile with tokio
less than 0.1.7, and they don't pass with tokio less than 0.1.8.  CI
didn't reveal the problem because new repos use the latest crates by
default.

asomers added a commit that referenced this pull request Dec 30, 2018

Fix the benchmark build after PR #10
The benchmark build was broken by PR #10, because we weren't yet testing
benches in CI.

asomers added a commit that referenced this pull request Dec 30, 2018

Fix the benchmark build after PR #10
The benchmark build was broken by PR #10, because we weren't yet testing
benches in CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment