search

radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
421 stars 39 forks source link

fix build #790

Closed Byron closed 2 years ago

Byron commented 2 years ago

This PR fixes what was reported in this issue: https://github.com/Byron/gitoxide/issues/221 .

Takeaways

History Here is what I encountered just to not forget: * update to latest version of radicle-link repo * run `cargo check` on master in repo root, all good * run `cargo test`, and it shows this error. Maybe `tokio` did a patch release with breaking changes? ``` error[E0603]: struct `UnixStream` is private --> rad-clib/src/keys/ssh/unix.rs:9:36 | 9 | use thrussh_agent::{client::tokio::UnixStream, Constraint}; | ^^^^^^^^^^ private struct | note: the struct `UnixStream` is defined here --> /Users/byron/.cargo/git/checkouts/thrussh-e462ef97472eec6c/d0248b3/thrussh-agent/src/client/tokio.rs:8:5 | 8 | use tokio::net::UnixStream; | ^^^^^^^^^^^^^^^^^^^^^^ ``` * switch to `xla/seen` branch * run `cargo check` in root and it shows the error above * run `cargo clean` just to be sure and `cargo check`. The error above re-appears * `cd librad` and run `cargo check` - it works. This is unexpected as it should fail. - run `cargo build` and it works which now was expected and so does `cargo test` * run `cargo build --workspace --all-features` which runs on CI and after fixing the above issue with `UnixStream` it does work Thus far, I am actually happy as it means that apparently `cargo smart-release` did what I expected it to do and bumped versions so that `cargo` won't pick them up.
Byron commented 2 years ago

Running cargo update brought in a few patch releases for git-* crates, these should be non-breaking. But that also pulled in git-hash 0.7!

   Updating git-actor v0.5.2 -> v0.5.3
    Updating git-config v0.1.6 -> v0.1.7
    Updating git-features v0.16.4 -> v0.16.5
      Adding git-hash v0.7.0
    Updating git-lock v1.0.0 -> v1.0.1
    Updating git-object v0.14.0 -> v0.14.1
    Updating git-tempfile v1.0.2 -> v1.0.3
    Updating git-url v0.3.3 -> v0.3.4
    Updating git-validate v0.5.2 -> v0.5.3

I would not be surprised if this is the culprit. And indeed, it is, now the issue is reproduced locally as well.

Byron commented 2 years ago

And the reason it worked for me is that I had an old Cargo.lock file locally which, unfortunately, now is lost as it's not checked in. That's probably intentional to always build with the latest versions of things, but it's also a risk as updates aren't controlled anymore. Maybe it's something to consider changing.

Nonetheless, git-features introduced a breaking change in a patch release by depending on a more recent version of git-hash (0.7 vs 0.6), which affects git-actor, which is ultimately clashing here. As far as I know, cargo smart-release does the so-called safety bumping based on transitive dependencies, but I will double-check or this could happen again.

This PR now contains only the fix.

Byron commented 2 years ago

Update: It's amazing to see that the safety-bumping feature apparently does work, so minor-bumping git-hash does indeed transitively minor bump all crates depending on it. Maybe there is a code-path that doesn't trigger this behaviour though.

     Running `target/debug/cargo-smart-release smart-release --update-crates-index --bump minor --no-dependencies --no-publish --no-tag --no-push -v git-hash`
[WARN ] With --no-tag enabled, changelog generation will be disabled as it relies on tags to segment commit history.
[INFO ] Updating crates-io index at '/Users/byron/.cargo/registry/index/github.com-1ecc6299db9ec823'
[WARN ] git-hash: The following tags were not encountered during commit graph traversal: refs/tags/git-hash-v0.4.0
[INFO ] WOULD modify existing changelog for 'git-hash'.
[INFO ] Using manifest version 0.6.0 of crate 'git-testtools' as it is sufficient to succeed latest published version 0.5.0.
[INFO ] Using current version 0.1.0 of crate object-access instead of bumped one 0.2.0.
[INFO ] Using current version 0.1.0 of crate diffing instead of bumped one 0.2.0.
[INFO ] Using current version 0.1.0 of crate object-access instead of bumped one 0.2.0.
[INFO ] Using current version 0.1.0 of crate diffing instead of bumped one 0.2.0.
[INFO ] Pending 'git-hash' manifest version update: "0.8.0"
[INFO ] Pending 'git-features' manifest version update: "0.17.0"
[INFO ] Pending 'git-features' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-testtools' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-ref' manifest version update: "0.9.0"
[INFO ] Pending 'git-ref' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-ref' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-ref' manifest dependencies update: 'git-object = "^0.15.0"' (from "^0.14.1")
[INFO ] Pending 'git-ref' manifest dependencies update: 'git-actor = "^0.6.0"' (from "^0.5.3")
[INFO ] Pending 'git-object' manifest version update: "0.15.0"
[INFO ] Pending 'git-object' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-object' manifest dependencies update: 'git-actor = "^0.6.0"' (from "^0.5.3")
[INFO ] Pending 'git-odb' manifest version update: "0.23.0"
[INFO ] Pending 'git-odb' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-odb' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-odb' manifest dependencies update: 'git-object = "^0.15.0"' (from "^0.14.1")
[INFO ] Pending 'git-odb' manifest dependencies update: 'git-pack = "^0.13.0"' (from "^0.12.0")
[INFO ] Pending 'git-pack' manifest version update: "0.13.0"
[INFO ] Pending 'git-pack' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-pack' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-pack' manifest dependencies update: 'git-object = "^0.15.0"' (from "^0.14.1")
[INFO ] Pending 'git-pack' manifest dependencies update: 'git-diff = "^0.11.0"' (from "^0.10.0")
[INFO ] Pending 'git-pack' manifest dependencies update: 'git-traverse = "^0.10.0"' (from "^0.9.0")
[INFO ] Pending 'git-diff' manifest version update: "0.11.0"
[INFO ] Pending 'git-diff' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-diff' manifest dependencies update: 'git-object = "^0.15.0"' (from "^0.14.1")
[INFO ] Pending 'git-traverse' manifest version update: "0.10.0"
[INFO ] Pending 'git-traverse' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-traverse' manifest dependencies update: 'git-object = "^0.15.0"' (from "^0.14.1")
[INFO ] Pending 'git-commitgraph' manifest version update: "0.6.0"
[INFO ] Pending 'git-commitgraph' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-commitgraph' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-protocol' manifest version update: "0.12.0"
[INFO ] Pending 'git-protocol' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-protocol' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-protocol' manifest dependencies update: 'git-transport = "^0.13.0"' (from "^0.12.0")
[INFO ] Pending 'git-protocol' manifest dev-dependencies update: 'git-packetline = "^0.12.0"' (from "^0.11.0")
[INFO ] Pending 'git-repository' manifest version update: "0.11.0"
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-hash = "^0.8.0"' (from "^0.7.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-ref = "^0.9.0"' (from "^0.8.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-object = "^0.15.0"' (from "^0.14.1")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-odb = "^0.23.0"' (from "^0.22.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-pack = "^0.13.0"' (from "^0.12.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-diff = "^0.11.0"' (from "^0.10.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-traverse = "^0.10.0"' (from "^0.9.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-protocol = "^0.12.0"' (from "^0.11.0")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-actor = "^0.6.0"' (from "^0.5.3")
[INFO ] Pending 'git-repository' manifest dependencies update: 'git-transport = "^0.13.0"' (from "^0.12.0")
[INFO ] Pending 'git-actor' manifest version update: "0.6.0"
[INFO ] Pending 'git-actor' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-packetline' manifest version update: "0.12.0"
[INFO ] Pending 'git-transport' manifest version update: "0.13.0"
[INFO ] Pending 'git-transport' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'git-transport' manifest dependencies update: 'git-packetline = "^0.12.0"' (from "^0.11.0")
[INFO ] Pending 'gitoxide-core' manifest version update: "0.12.0"
[INFO ] Pending 'gitoxide-core' manifest dependencies update: 'git-pack-for-configuration-only = "^0.13.0"' (from "^0.12.0")
[INFO ] Pending 'gitoxide-core' manifest dependencies update: 'git-commitgraph = "^0.6.0"' (from "^0.5.0")
[INFO ] Pending 'gitoxide-core' manifest dependencies update: 'git-repository = "^0.11.0"' (from "^0.10.0")
[INFO ] Pending 'traversal' manifest version update: "0.2.0"
[INFO ] Pending 'traversal' manifest dependencies update: 'git-repository = "^0.11.0"' (from "^0.10.0")
[INFO ] Pending 'cargo-smart-release' manifest version update: "0.5.0"
[INFO ] Pending 'cargo-smart-release' manifest dependencies update: 'git-repository = "^0.11.0"' (from "^0.10.0")
[INFO ] Pending 'gitoxide' manifest version update: "0.10.0"
[INFO ] Pending 'gitoxide' manifest dependencies update: 'git-features = "^0.17.0"' (from "^0.16.5")
[INFO ] Pending 'gitoxide' manifest dependencies update: 'git-repository = "^0.11.0"' (from "^0.10.0")
[INFO ] Pending 'gitoxide' manifest dependencies update: 'git-transport-for-configuration-only = "^0.13.0"' (from "^0.12.0")
[INFO ] Pending 'gitoxide' manifest dependencies update: 'gitoxide-core = "^0.12.0"' (from "^0.11.0")
[INFO ] WOULD persist changes to 19 manifests and 1 changelogs with: "Adjusting changelogs prior to release of git-hash v0.8.0, safety bump 17 crates\n\nSAFETY BUMP: git-features v0.17.0, git-ref v0.9.0, git-object v0.15.0, git-odb v0.23.0, git-pack v0.13.0, git-diff v0.11.0, git-traverse v0.10.0, git-commitgraph v0.6.0, git-protocol v0.12.0, git-repository v0.11.0, git-actor v0.6.0, git-packetline v0.12.0, git-transport v0.13.0, gitoxide-core v0.12.0, traversal v0.2.0, cargo-smart-release v0.5.0, gitoxide v0.10.0"
Byron commented 2 years ago

The windows build failure seems to be unrelated to this change.

FintanH commented 2 years ago

This looks good to me :+1: No worries about the Windows failure, our ssh-agent work isn't compatible.

Unfortunately, the advisory step is failing because of an issue with the time package[0]. We can't fix this locally since chrono is using it transitively and there's a PR that doesn't seem to be getting merged[1].

kim commented 2 years ago

That's probably intentional to always build with the latest versions of things, but it's also a risk as updates aren't controlled anymore. Maybe it's something to consider changing.

This is on you, Herr Kollege: cargo only considers the lock file when building from the source repository/workspace. This means that your CI will not catch issues with the smartness of the release tooling, and instead is guinea pigging downstream consumers. That’s not okay.

Byron commented 2 years ago

@FintanH

Unfortunately, the advisory step is failing because of an issue with the time package[0]. We can't fix this locally since chrono is using it transitively and there's a PR that doesn't seem to be getting merged[1].

Thanks for the heads-up, I would have probably have missed it and am puzzled as to how this happens now. Was another dependency updated recently so it's merely a coincidence? time 1 was once a dependency of gitoxide but was removed some time ago.

@kim

This is on you, Herr Kollege: cargo only considers the lock file when building from the source repository/workspace. This means that your CI will not catch issues with the smartness of the release tooling, and instead is guinea pigging downstream consumers. That’s not okay.

I didn't mean to come across as deflective, and if there is anyone to blame than it is me 🤦‍♂️. Breaking the downstream or using them as test-subjects isn't OK to my mind either, so I spent the weekend on https://github.com/Byron/gitoxide/pull/223 to fix cargo smart-release once and for all. I know, definitely last words, but it's capable of performing safety-bumps now as it should have been the first time around. Without it, the fight is already lost. With it, there is a chance as long as I am sprinkling in these conventional messages into the git history to indicate breakage.

As an additional safety measure, I will try to build radicle-link locally after the next release.

This means that your CI will not catch issues with the smartness of the release tooling[…]

Da stehe ich auf dem Schlauch. From all I know, Cargo.lock is picked up by CI as well and neatly pins dependencies. Only when installing crates an extra flag is needed to actually use the lock file, like cargo install --locked …, but for CI it was of great value for me and provides a safe-state of dependencies.

FintanH commented 2 years ago

Thanks for the heads-up, I would have probably have missed it and am puzzled as to how this happens now. Was another dependency updated recently so it's merely a coincidence? time 1 was once a dependency of gitoxide but was removed some time ago.

I think it just hit the advisory database and so it's getting picked up now.

Byron commented 2 years ago

Thanks for the heads-up, I would have probably have missed it and am puzzled as to how this happens now. Was another dependency updated recently so it's merely a coincidence? time 1 was once a dependency of gitoxide but was removed some time ago.

I think it just hit the advisory database and so it's getting picked up now.

Right, they talk about that here: https://github.com/time-rs/time/issues/293. Looks like everyone with chrono using cargo deny is broken.

kim commented 2 years ago

This means that your CI will not catch issues with the smartness of the release tooling[…]

Da stehe ich auf dem Schlauch. From all I know, Cargo.lock is picked up by CI as well and neatly pins dependencies. Only when installing crates an extra flag is needed to actually use the lock file, like cargo install --locked …, but for CI it was of great value for me and provides a safe-state of dependencies.

The problem is this: you are publishing a library (amongst other things, but that's from our perspective). We are pulling that library from the internet, and cargo will not consider the lock file of your library (not even cargo install does!). Since your CI considers the lock file, the dependency resolution mechanism of cargo will not kick in when you build -- it will try as hard as it can to stick to the exact versions in the lock file. This means that you are not aware of any dependency resolution issues until someone downstream tells you.

Pinning dependencies is useful when you deploy things (like, a webservice or a distribution package) because you want to be able to reproduce the build. It is not useful when you publish libraries, because you force everyone to use your exact dependency closure (which either leads to incompatibilities or duplicate versions in the dependency tree, or both).

Of course, if your CI always resolves dependencies (ie. no lock file), you may get a build failure when no code of your own has changed. In this case, you want to know what cargo resolved, so you can reproduce the issue.

Because cargo cannot easily be convinced to ignore the lock file (and we also don't want robots to have write access to our source code repository), the workaround we came up with is to publish the lock file cargo generates as a build artifact.

We also keep executables in a separate workspace, so we can version control lock files for them. Considering cargo install doesn't consider lock files, that's still not ideal, but a good enough compromise for now.

Byron commented 2 years ago

The problem is this: you are publishing a library (amongst other things, but that's from our perspective). We are pulling that library from the internet, and cargo will not consider the lock file of your library (not even cargo install does!). Since your CI considers the lock file, the dependency resolution mechanism of cargo will not kick in when you build -- it will try as hard as it can to stick to the exact versions in the lock file. This means that you are not aware of any dependency resolution issues until someone downstream tells you.

There we didn't have a misunderstanding.

Of course, if your CI always resolves dependencies (ie. no lock file), you may get a build failure when no code of your own has changed. In this case, you want to know what cargo resolved, so you can reproduce the issue.

Because cargo cannot easily be convinced to ignore the lock file (and we also don't want robots to have write access to our source code repository), the workaround we came up with is to publish the lock file cargo generates as a build artifact.

Now that's very interesting, and a perfect explanation for why no lock file is checked in to the repository. It's interesting that where cargo should actually use a lock, during installation of a binary to make breakage impossible, it won't unless --locked is used which I always forget.

We also keep executables in a separate workspace, so we can version control lock files for them. Considering cargo install doesn't consider lock files, that's still not ideal, but a good enough compromise for now.

For me there is probably something to be learned here, but no matter how I turn it, I keep wanting reproducible everything and controlled upgrades just because from experience conventional package resolution based on semver can't be trusted and there is always someone out to break you… 🤦‍♂️.

On that note, smart-release is done and I have just updated this commit to the latest published version created just to validate that 'safety bumps' are indeed working now.

With this the downstream will be safe for good 😅!

PS: only windows tests fail now for other reasons.

kim commented 2 years ago

For me there is probably something to be learned here, but no matter how I turn it, I keep wanting reproducible everything and controlled upgrades just because from experience conventional package resolution based on semver can't be trusted and there is always someone out to break you… 🤦‍♂️.

Yes, and that's why you want to know asap when this happens. Again, you are releasing libraries, and you don't control your user's dependency tree, only yours.

Byron commented 2 years ago

Yes, and that's why you want to know asap when this happens. Again, you are releasing libraries, and you don't control your user's dependency tree, only yours.

Agreed, fingers crossed the technology will help avoid this in future. Instead, when making a release with breaking changes I want to proactively submit a PR with an upgrade before anything breaks.

My plan with this PR is to check in from time to time and rebase to see if windows will be green, but feel free to ping me once windows is working again.

kim commented 2 years ago

Will include in #791