rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

yamux 0.10.1, set_split_send_size

Open folex opened this issue 3 years ago • 6 comments

  • Update yamux to 0.10.1
  • Add set_split_send_size to YamuxConfig

As I understand, it affects #2461.

P.S. I'm kinda surprised you don't commit Cargo.lock, is there a reason for that? From my PoV, it makes CI builds unpredictable and generates hard-to-investigate dependency bugs.

folex avatar Apr 06 '22 08:04 folex

This is a best practice as a library. Which dependency bug did you run into?

We're running build on nightly, so sometimes cargo update brings compilation errors. Without Cargo.lock commited, every build on CI is basically preceded by cargo update.

It's the usual practice to commit .lock files in JS and Haskell. But in Rust, Cargo book suggests against it for library authors. And it makes little sense to me, cuz Cargo.lock only affects CI/local builds, it doesn't affect publishing to crates.io.

So I'm curious on your reasons to do that :)

folex avatar Apr 08 '22 16:04 folex

@mxinden anything else I can do to merge this PR?

folex avatar Apr 19 '22 09:04 folex

It's the usual practice to commit .lock files in JS and Haskell. But in Rust, Cargo book suggests against it for library authors. And it makes little sense to me, cuz Cargo.lock only affects CI/local builds, it doesn't affect publishing to crates.io.

So I'm curious on your reasons to do that :)

No particular reasoning on my end other than following the best practice. I guess it does reduce the maintenance work a bit as one does not need to keep it up to date, e.g. via Dependabot.

mxinden avatar Apr 23 '22 09:04 mxinden

@mxinden anything else I can do to merge this PR?

I still don't understand why you need to configure this flag. Can you expand on that once more? I am reluctant to add a configuration flag just for the sake of debugging.

mxinden avatar Apr 23 '22 09:04 mxinden

@mxinden anything else I can do to merge this PR?

I still don't understand why you need to configure this flag. Can you expand on that once more? I am reluctant to add a configuration flag just for the sake of debugging.

I needed it to overcome the issue as I described. I've no other use-case. In general, I feel that it should be possible to configure underlying mechanisms without going through rather hard process of forking the whole libp2p.

folex avatar Apr 25 '22 10:04 folex

This is a best practice as a library. Which dependency bug did you run into?

We're running build on nightly, so sometimes cargo update brings compilation errors. Without Cargo.lock commited, every build on CI is basically preceded by cargo update.

It's the usual practice to commit .lock files in JS and Haskell. But in Rust, Cargo book suggests against it for library authors. And it makes little sense to me, cuz Cargo.lock only affects CI/local builds, it doesn't affect publishing to crates.io.

So I'm curious on your reasons to do that :)

As library authors, we should strive for having our library work with as many version combinations as possible. Locking down dependencies to specific patch versions is not helpful in that regard.

By default, cargo uses the caret modifier for dependency versions, meaning it will only build against that exact version or newer patch versions.

Within semver, there should never be breaking changes within patch versions. I think it is useful to recommend against tracking Cargo.lock in Git for library authors because it has the ecosystem-wide effect that breaking API changes in patch versions are extremely rare and when they happen, they are detected very early. Libraries tend to have less dependencies so it is also easier for library authors to work out, which dependency is the problem instead of putting this burden onto the application projects.

thomaseizinger avatar Jun 03 '22 06:06 thomaseizinger

This pull request has merge conflicts. Could you please resolve them @folex? 🙏

mergify[bot] avatar Jan 02 '23 00:01 mergify[bot]