fix build #790
fix build #790
Conversation
|
Running I would not be surprised if this is the culprit. And indeed, it is, now the issue is reproduced locally as well. |
|
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, This PR now contains only the fix. |
|
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. |
|
The windows build failure seems to be unrelated to this change. |
|
This looks good to me 👍 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 |
|
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.
|
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?
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 GitoxideLabs/gitoxide#223 to fix As an additional safety measure, I will try to build
Da stehe ich auf dem Schlauch. From all I know, |
I think it just hit the advisory database and so it's getting picked up now. |
Right, they talk about that here: time-rs/time#293. Looks like everyone with chrono using cargo deny is broken. |
|
> 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.
|
This assures that only `git-hash:0.8.0` is used in the dependency tree. The latter should not have been pulled in transitively, which points to an issue with `cargo smart-release` which was built to prevent this. Signed-off-by: Sebastian Thiel <[email protected]>
There we didn't have a misunderstanding.
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
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, With this the downstream will be safe for good 😅! PS: only windows tests fail now for other reasons. |
|
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.
|
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. |
|
Will include in #791 |
This PR fixes what was reported in this issue: GitoxideLabs/gitoxide#221 .
Takeaways
git-featureswas introducing a breaking update togit-hash(v0.6 -> v0.8) in a patch release. This should not be possible ascargo smart-releaseis supposed to prevent this kind of thing. I will definitely look into it as this is a key-requirement to preventing issues like these.Cargo.lockcaused surprising build results, and since binaries are built here it should probably be tracked by git and updated in a controlled fashion.History
Here is what I encountered just to not forget:
cargo checkon master in repo root, all goodcargo test, and it shows this error. Maybetokiodid a patch release with breaking changes?xla/seenbranchcargo checkin root and it shows the error abovecargo cleanjust to be sure andcargo check. The error above re-appearscd libradand runcargo check- it works. This is unexpected as it should fail.cargo buildand it works which now was expected and so doescargo testcargo build --workspace --all-featureswhich runs on CI and after fixing the above issue withUnixStreamit does workThus far, I am actually happy as it means that apparently
cargo smart-releasedid what I expected it to do and bumped versions so thatcargowon't pick them up.