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

Review cargo-smart-release safety bumping feature #222

Closed
7 tasks done
Byron opened this issue Oct 16, 2021 · 1 comment
Closed
7 tasks done

Review cargo-smart-release safety bumping feature #222

Byron opened this issue Oct 16, 2021 · 1 comment

Comments

@Byron
Copy link
Member

Byron commented Oct 16, 2021

Even though it generally works, it won't correctly figure out which crates to release in the light of a safety bump, nor does it correctly bump about-to-be-released crates that are safety bumped themselves.

To reproduce:

  • add a breaking change in git-hash
  • add any non-breaking change in git-repository
  • try to release git-repository

Even though it will call out a lot of safety bumps, it will fail to:

  • minor bump git-repository even though it should be affected by the safety bump caused by git-hash
  • release all crates that have been minor-bumped as triggered by git-hash that are in the dependency chain of git-repository

The above would cause publish failure as git-repository depends on safety-bumped version of its dependencies which have not been published yet.

Getting this right probably meant to alter how the dependency processing works a lot, it appears the dependency-checking should have access to the safety-bumped crates, instead it only looks at changes in the git-history. Alternatively, the skipped-crates could be picked up again by the safety bumping, allowing them to be re-added now that a bump is desired and they were determined to be in the dependency chain of about-to-be-published crates.

Probably the cause of #221, which is downstream breakage due to safety bumps not being applied even though they should have.

Tasks

  • the fix
  • order of crates is wrong if breaking-safety bumps are inserted. Need to reorder according to dependency order somehow or the publish will fail.
  • unchanged crates that are marked for publishing might not actually be published and it fails to recognize breaking changes should publish it. Or am I getting this wrong?
  • forced releases are possible now, meaning that there is no change but a breaking change is happening in dependencies. This could be fixed by doing a separate commit for changelogs after manifest updates, always giving them one commit. Or such an update could just be handled specifically. - it's really about it refusing to publish unchanged crates despite them seeing breaking changes. It's about dependency breaking change resolution.
  • it's still possible to have breaking changes affect a crate that should be published, but it won't be published as it had no changes originally. It's about resolution - seems to work now.
  • smart-release cargo-smart-release gitoxide refuses to release gitoxide, even though cargo smart-release wants to to it just fine.
  • publish result seems dependent on input package order, see gitoxide cargo-smart-release vs the inverse.
@Byron Byron added the bug label Oct 16, 2021
@Byron Byron mentioned this issue Oct 16, 2021
Byron added a commit that referenced this issue Oct 16, 2021
…ependencies` (#222)

This leads to easier usage in the common case and helps avoid confusion
when --dependencies is used and it's not picking up safety bumps on the
way, like `smart-release` would.
Byron added a commit that referenced this issue Oct 17, 2021
Switch to using packages cosnistently and use package.id for
comparisons.

This is the basis for cleaning up dependency and version handling
to allow fixing the existing issues.
Byron added a commit that referenced this issue Oct 17, 2021
Prepare to push everything into data, and do the logging elsewhere, when
needed.
Byron added a commit that referenced this issue Oct 17, 2021
Make enough context available for allowing version bumping during
dependency resolution.
Byron added a commit that referenced this issue Oct 17, 2021
Presentation is now entirely data driven, instead of being mixed into
the functions that figure out what's going on.

This helps to keep things clean and lean to facilitate adding version
handling on top.
Byron added a commit that referenced this issue Oct 17, 2021
…which opens up much improved version handling and reporting, one line
per crate and not multiple.
Byron added a commit that referenced this issue Oct 17, 2021
…ntout (#222)

Goal is to prepare the data for adjustment and transformation as
safety-bumps are determined in another pass.
Byron added a commit that referenced this issue Oct 17, 2021
Version-logic is now entirely cleaned up and stripped to pure logic,
making it easier to understand and way more testable.
Byron added a commit that referenced this issue Oct 17, 2021
But it doesn't capture that this can happen to provided or dependent
crates.
Byron added a commit that referenced this issue Oct 17, 2021
Now safety-bumps are indicated by the causing package.
Byron added a commit that referenced this issue Oct 17, 2021
Byron added a commit that referenced this issue Oct 17, 2021
Note that we use it like it's not lower than debug, just to have another
color and make important messages pop more.
Byron added a commit that referenced this issue Oct 17, 2021
Use semver::Version everywhere instead of str/String. Strongly typed
things are definitely better, don't know how that happened even.
Byron added a commit that referenced this issue Oct 17, 2021
Byron added a commit that referenced this issue Oct 17, 2021
Another data-structure change to be more fitting. Remove a lot of
printing as well, it could be recreated once there is need.
Byron added a commit that referenced this issue Oct 17, 2021
as they would now show up in the journey tests, making
snapshot testing unusable.
Byron added a commit that referenced this issue Oct 17, 2021
As the default is to do multi-crate releases and now having to deal
with single-create releases reduces maintenance burden.

The solution to this problem is to not specify versions in
dev-dependencies to workspace crates.

We also don't check for this anymore, which might be re-added
at some point if there is demand.
Byron added a commit that referenced this issue Oct 17, 2021
Byron added a commit that referenced this issue Oct 18, 2021
…but realize that forward traversal while rebuilding the whole list
might be better. Or maybe not? Things need to stay ordered after all.

Maybe starting from where things may break, and keeping edits
separately, is more like it.
Byron added a commit that referenced this issue Oct 18, 2021
Otherwise we might version-bump crates that have nothing to do
with the breaking create, but are merely in the dependency tree of
the starting crate.
Byron added a commit that referenced this issue Oct 19, 2021
Byron added a commit that referenced this issue Oct 19, 2021
As it's unclear how that should be or what happens in reality,
let's watch for logs and improve it when the need arises.
Byron added a commit that referenced this issue Oct 19, 2021
Previously the flag had no effect and changelogs would always be
generated, possibly stopping the release as at least one of them
needed manual work.
Byron added a commit that referenced this issue Oct 19, 2021
Previously the ordering of crate for release might not have been
correct due to this issue that is now fixed.

We need depth-first traversals and previously it would extend skipped
dependencies, effectively putting them into their own ordering.

Previously it would restore that ordering, but not anymore, causing
this bug that was entirely unnecessary.
Byron added a commit that referenced this issue Oct 19, 2021
When a user-selected crate was initially discarded as it had no change,
it may still be picked up again in case it is affected by breaking
changes.

When that happens, it would previously never partake in the release,
despite the breaking change, but will do now if the user provided it.
Byron added a commit that referenced this issue Oct 19, 2021
…rect order (#222)

The problem here is that even though we can turn non-publishable breaks
into publishable ones without loosing information, they will not be in
the correct order.

The solution is to merge dependency trees instead of clearing them with
weird logic.
Byron added a commit that referenced this issue Oct 19, 2021
Byron added a commit that referenced this issue Oct 19, 2021
separate changelog related work more from manifest code, where feasible
Byron added a commit that referenced this issue Oct 19, 2021
cleanup manifest to separate everything into smaller function.
That includes some overhead, but makes what's happening there much more
graspable.
Byron added a commit that referenced this issue Oct 19, 2021
@Byron Byron closed this as completed Oct 19, 2021
Byron added a commit that referenced this issue Oct 19, 2021
Byron added a commit that referenced this issue Oct 19, 2021
When there was no change in the src/ directory of the top-level crate,
the dependency resolution would not be able to auto-bump the version
as no change occurred, but another part would usually detect a change
as it wasn't confined to the top-level src/ directory.

This could lead to a panic as an invariant wasn't upheld.

This was fixed by letting both parts agree to use the src/ directory
to determine changes of the top-level directory, and by making panics
impossible while improving the messaging around this state should it
still occur. The latter is rough, probably rare, but usable.
Byron added a commit that referenced this issue Oct 19, 2021
Previously even unchanged crates would trigger workspace crates
to be recorded for manifest changes.

Now only crates that are to receive manifest changes will be triggering
this.
@Byron
Copy link
Member Author

Byron commented Oct 19, 2021

@epage I remember you mentioning a shared dependency engine of sorts and after this arduous rewrite of exactly that it's now a step closer to being possible.

  • traverse.rs
    • analyse the dependency graph and produce a flat list of data about each crate in publishable order (even if many items in the list won't actually be published)
  • present_dependencies()
    • takes the data from above and makes it understandable to the user
    • potentially abort if some invariant isn't met
  • manifest and changelog adjustments
    • after presenting, most of the work is done there to ultimately adjust all manifests and changelogs

Now the same data structure is used by the changelog subcommand so it knows exactly which crates will be published, and hence need changelogs generated. After a lot of back and forth and polishing I finally feel good about the logic in there (as in: a human being can understand it), so I'd be glad to hear what you think if you have the time.

Byron added a commit that referenced this issue Oct 19, 2021
This is more atomic and prevents loosing all github releases if one
publish fails later on.
Byron added a commit that referenced this issue Oct 19, 2021
That way, tags don't remain unpushed despite having been created
successfully, just because one crate later in the publishing
process fails.
Byron added a commit that referenced this issue Oct 19, 2021
Byron added a commit that referenced this issue Oct 19, 2021
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

No branches or pull requests

1 participant