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 sometimes-unused dependencies #603

Merged
merged 3 commits into from
Jan 17, 2022
Merged

Fix sometimes-unused dependencies #603

merged 3 commits into from
Jan 17, 2022

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Oct 15, 2021

Analysis done with cargo-udeps.

@jplatte
Copy link
Contributor Author

jplatte commented Jan 17, 2022

Rebased. Ping @davidpdrsn can you review this?

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I think this looks good but would like someone else to +1 as well. cc @hawkw @olix0r

@olix0r
Copy link
Collaborator

olix0r commented Jan 17, 2022

CI fails because this change introduces unsafe code into a module that uses forbid(unsafe). I'd prefer to revert that part of the change (i.e. continue using pin-project-lite) so that we can avoid unsafe code in this crate.

pin-project-lite is almost definitely going to be in your dependency tree if you're using tokio.

@davidpdrsn
Copy link
Member

I believe the issue is that moving Either to pin-project-lite is a breaking change.

@olix0r
Copy link
Collaborator

olix0r commented Jan 17, 2022

I believe the issue is that moving Either to pin-project-lite is a breaking change.

That's a little surprising to me. Does pin-project-lite change the API surface when compared to pin-project? Howso?

If we can't use lite, I'd still prefer continuing use of pin-project if the alternative is adding unsafe code to tower -- it just expands the audit surface area. We're due for a breaking change sometime soon, I think, anyway. So we should get an issue to switch to use lite then if we really can't change now.

@jplatte
Copy link
Contributor Author

jplatte commented Jan 17, 2022

The issue with using pin-project-lite for Either is that it doesn't support tuple structs or tuple variants in enums. I don't think you would want to change ``Either::A(A)toEither::A { inner: A }`, right? In that case, the only options are to keep the `pin-project` dependency or accept the tiny bit of unsafe code. IMHO, the latter is better.

I'll separate that change from this PR though given that the other changes seem less controversial.

It is no longer recommended to set it:
rust-lang/api-guidelines#230
Path dependencies are not problematic for publishing as long as a
version is also specified.
@olix0r
Copy link
Collaborator

olix0r commented Jan 17, 2022

@jplatte That makes sense. Thanks for splitting out the change. We'll continue the conversation in #629

@olix0r olix0r merged commit d0d8707 into tower-rs:master Jan 17, 2022
hawkw added a commit that referenced this pull request Feb 16, 2022
# 0.4.12 (February 16, 2022)

### Fixed

- **hedge**, **load**, **retry**: Fix use of `Instant` operations that
  can panic on platforms where `Instant` is not monotonic ([#633])
- Disable `attributes` feature on `tracing` dependency ([#623])
- Remove unused dependencies and dependency features with some feature
  combinations ([#603], [#602])
- **docs**: Fix a typo in the RustDoc for `Buffer` ([#622])

### Changed

- **hedge**: Updated `hdrhistogram` dependency to v7.0 ([#602])
- Updated `tokio-util` dependency to v0.7 ([#638])

[#633]: #633
[#623]: #623
[#603]: #603
[#602]: #602
[#622]: #622
[#638]: #638
hawkw added a commit that referenced this pull request Feb 16, 2022
* chore: prepare to release v0.4.12

# 0.4.12 (February 16, 2022)

### Fixed

- **hedge**, **load**, **retry**: Fix use of `Instant` operations that
  can panic on platforms where `Instant` is not monotonic ([#633])
- Disable `attributes` feature on `tracing` dependency ([#623])
- Remove unused dependencies and dependency features with some feature
  combinations ([#603], [#602])
- **docs**: Fix a typo in the RustDoc for `Buffer` ([#622])

### Changed

- Updated minimum supported Rust version (MSRV) to 1.49.0.
- **hedge**: Updated `hdrhistogram` dependency to v7.0 ([#602])
- Updated `tokio-util` dependency to v0.7 ([#638])

[#633]: #633
[#623]: #623
[#603]: #603
[#602]: #602
[#622]: #622
[#638]: #638

* add msrv
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.

3 participants