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

Consider review of dependencies #140

Closed
bstrie opened this issue Jul 18, 2019 · 3 comments · Fixed by #627
Closed

Consider review of dependencies #140

bstrie opened this issue Jul 18, 2019 · 3 comments · Fixed by #627
Assignees
Labels
dependencies v1.0 Issues which need to be handled for a v1.0 release

Comments

@bstrie
Copy link
Contributor

bstrie commented Jul 18, 2019

Unstructured musings:

  • the parking_lot crate is potentially being integrated into libstd (though that will take a while and I don't want to guess at an ETA), though if we're not certain that we need parking_lot for speed then we could benchmark it and see if it's necessary for our use case
  • our remaining uses of tower-web should be switched out for warp
  • in a recent release some functionality of the failure crate was upstreamed into libstd, so we may be able to reduce the need for that dependency
  • several of our crates are depending on chrono, which may be possible to jettison depending on what functionality specifically we're using
  • regarding the rand crate, I'm curious what functionality of rand that we require and whether we're using random numbers in a context where the quality of the PRNG matters
  • I want to investigate what the regex crate is being used for and see whether a non-regex solution might be preferable for those
  • I want to comb through some of the packages I've never heard of before and make sure they're still maintained
  • likewise if we have packages pulling in different versions of the same transitive dependency then it might be useful to submit PRs to projects to see if we can get them all to upgrade to the same version
  • quick-error in interledger-packet can be removed by refactoring the Error type
@emschwartz
Copy link
Member

the futures crate might often be able to be replaced by the recently stabilized std::future module, depending on how much functionality is used

Definitely worth investigating. Want to add that to your queue?

the parking_lot crate is potentially being integrated into libstd (though that will take a while and I don't want to guess at an ETA), though if we're not certain that we need parking_lot for speed then we could benchmark it and see if it's necessary for our use case

The other advantage is that it doesn't return a Result so you don't need to have .unwrap all over the place

we've got a lot of different crates doing http like reqwest and hyper and tower-web, so depending on our needs we may want to consolidate here

reqwest and tower-web both use hyper so we should just make sure we have consistent versions of the first two and that they're pulling in the same version of hyper

in a recent release some functionality of the failure crate was upstreamed into libstd, so we may be able to reduce the need for that dependency

👍

several of our crates are depending on chrono, which may be possible to jettison depending on what functionality specifically we're using

Worth looking into. Right now I believe no crate exposes chrono types in its API but it is useful for parsing timestamps from ILP packets (not sure how much code it would be to replicate the specific behavior we need)

regarding the rand crate, I'm curious what functionality of rand that we require and whether we're using random numbers in a context where the quality of the PRNG matters

A very good question to ask. I'm pretty sure we're using ring everywhere the PRNG matters but worth auditing to make sure that's the case

I want to investigate what the regex crate is being used for and see whether a non-regex solution might be preferable for those

👍

I want to comb through some of the packages I've never heard of before and make sure they're still maintained

👍 👍

likewise if we have packages pulling in different versions of the same transitive dependency then it might be useful to submit PRs to projects to see if we can get them all to upgrade to the same version

👍 👍

@bstrie
Copy link
Contributor Author

bstrie commented Sep 3, 2019

Noting that regex now has the ability to turn off many features in order to reduce dependencies and improve compilation time: rust-lang/regex#613

@bstrie
Copy link
Contributor Author

bstrie commented Feb 7, 2020

#627 gets us about as close as we could be reasonably expected to do right now, barring the exception tracked in #626.

At the maximum, above-and-beyond-the-call-of-duty end of things, we could take note of the fact that we do still have some duplicated transitive dependencies thanks to some external crates that have not bumped their versions in a while, as shown by the following:

$ git grep "name =" Cargo.lock | uniq -c | grep -v "1 Cargo.lock" | sort -r
   3 Cargo.lock:name = "rand_core"
   2 Cargo.lock:name = "winapi"
   2 Cargo.lock:name = "want"
   2 Cargo.lock:name = "version_check"
   2 Cargo.lock:name = "url"
   2 Cargo.lock:name = "tungstenite"
   2 Cargo.lock:name = "tokio"
   2 Cargo.lock:name = "smallvec"
   2 Cargo.lock:name = "rand"
   2 Cargo.lock:name = "percent-encoding"
   2 Cargo.lock:name = "parking_lot_core"
   2 Cargo.lock:name = "parking_lot"
   2 Cargo.lock:name = "nom"
   2 Cargo.lock:name = "log"
   2 Cargo.lock:name = "input_buffer"
   2 Cargo.lock:name = "idna"
   2 Cargo.lock:name = "hyper"
   2 Cargo.lock:name = "http-body"
   2 Cargo.lock:name = "http"
   2 Cargo.lock:name = "h2"
   2 Cargo.lock:name = "futures"
   2 Cargo.lock:name = "crossbeam-utils"
   2 Cargo.lock:name = "bytes"
   2 Cargo.lock:name = "base64"
   2 Cargo.lock:name = "autocfg"

Some of this would likely be addressed by fixing #626, but others would involve figuring out where the duplicated dependencies are coming from (it's easy enough to check Cargo.lock to figure this out), sending PRs to those projects updating their dependencies, and then waiting for a new release. Alternatively if we wait long enough some of that may happen without our intervention (especially for futures-related crates).

In any case, I'll consider this closed when #627 lands.

@bstrie bstrie moved this from In progress to Waiting Review / Response in Towards a v1.0 Feb 7, 2020
Towards a v1.0 automation moved this from Waiting Review / Response to Done Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies v1.0 Issues which need to be handled for a v1.0 release
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants