Add SemVer compatibility checks to CI

[tnull]

We recently introduced release branches that need to remain backwards compatible. However, even small changes to item visibility during backporting fixes might introduce SemVer violations (see https://doc.rust-lang.org/cargo/reference/semver.html#change-categories for a list of changs that would be considered major/minor).

To make sure we don't accidentally introduce such changes in the backports, we here add a new semver-checks CI job that utilizes cargo-semver-checks
(https://github.com/obi1kenobi/cargo-semver-checks), and have it run on any push or pull requests towards anything else but main/master (i.e., all feature branches to come).

Example output from a local run:

lightning-invoice> cargo semver-checks --default-features
    Building lightning-invoice v0.32.0 (current)
       Built [   0.329s] (current)
     Parsing lightning-invoice v0.32.0 (current)
      Parsed [   0.005s] (current)
     Parsing lightning-invoice v0.32.0 (baseline, cached)
      Parsed [   0.006s] (baseline)
    Checking lightning-invoice v0.32.0 -> v0.32.0 (no change)
     Checked [   0.010s] 107 checks: 105 pass, 2 fail, 0 warn, 0 skip

--- failure enum_tuple_variant_field_added: pub enum tuple variant field added ---

Description:
An enum's exhaustive tuple variant has a new field, which has to be included when constructing or matching on this variant.
        ref: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_tuple_variant_field_added.ron

Failed in:
  field 1 of variant Bolt11ParseError::InvalidSliceLength in /Users/erohrer/workspace/rust-lightning/lightning-invoice/src/lib.rs:106
  field 2 of variant Bolt11ParseError::InvalidSliceLength in /Users/erohrer/workspace/rust-lightning/lightning-invoice/src/lib.rs:106

--- failure type_mismatched_generic_lifetimes: type now takes a different number of generic lifetimes ---

Description:
A type now takes a different number of generic lifetime parameters. Uses of this type that name the previous number of parameters will be broken.
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/type_mismatched_generic_lifetimes.ron
Failed in:
  Enum Bolt11InvoiceDescription (1 -> 0 lifetime params) in /Users/erohrer/workspace/rust-lightning/lightning-invoice/src/lib.rs:243

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [   0.895s] lightning-invoice
exit 1

[tnull]

Currently we just check with default-features. Looking for a concept ACK here before going down the road of adding all 'interesting' feature combinations per crate (since our features are not additive, we can't just check on all-features).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (86308e1) to head (99fd11c).
Report is 101 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3560      +/-   ##
==========================================
+ Coverage   88.40%   89.61%   +1.21%     
==========================================
  Files         149      149              
  Lines      113874   122216    +8342     
  Branches   113874   122216    +8342     
==========================================
+ Hits       100674   109529    +8855     
+ Misses      10690    10294     -396     
+ Partials     2510     2393     -117     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TheBlueMatt]

Nice! LGTM, why is this draft? Does this need to be backported to the 0.1 branch as well?

[tnull]

Nice! LGTM, why is this draft? Does this need to be backported to the 0.1 branch as well?

Well, for one I wanted to get a concept ACK before adding more (per-crate) features besides default-features. Also, there currently is an error regarding lightning-dns-resolution as its version 0.1.0 has been yanked (i.e., it looks for 0.1.0 as a baseline). We'll need to figure out what a good workaround is. I think there is a way to specify the baseline manually, otherwise we could consider re-publishing 0.1.0 possibly?

[TheBlueMatt]

Oh, is it possible to, on PRs tagged "backport X" we cherry-pick it onto brach X and then run the semver check? That, I guess is really what we want.

Also, there currently is an error regarding lightning-dns-resolution as its version 0.1.0 has been yanked (i.e., it looks for 0.1.0 as a baseline).

Can we make 0.2 the baseline?

[tnull]

Oh, is it possible to, on PRs tagged "backport X" we cherry-pick it onto brach X and then run the semver check? That, I guess is really what we want.

IIUC, shouldn't this already be done here as it will run on any PRs towards the 0.1 branches, etc?

Can we make 0.2 the baseline?

Ah, IIUC, it seems it defaults to use the version prior to the one in Cargo.toml as the baseline. So on current main it simply gets confused since we haven't landed #3546 yet. After that it seems to work fine, as then it will take 0.2.0 as the (minor) baseline for 0.3.0+git. Note that this means we could also consider running it generally on all PRs, even including PRs towards main post-#3546. Should we do that?

[tnull]

Pushed a fixup with additional checks for crate features (let me know if I forgot something important). Undrafting.

[tnull]

Btw, also opened a corresponding PR on the LDK Node side: lightningdevkit/ldk-node#445

[TheBlueMatt]

IIUC, shouldn't this already be done here as it will run on any PRs towards the 0.1 branches, etc?

Right, but what we've been doing so far is opening a PR to main first with the backport X label, then opening a multi-commit backport rollup PR that. Catching the error there is good, but catching the error earlier would be better.

[tnull]

Right, but what we've been doing so far is opening a PR to main first with the backport X label, then opening a multi-commit backport rollup PR that. Catching the error there is good, but catching the error earlier would be better.

I don't think what you're describing is possible then as many of the backport-labeled PRs do not cleanly rebase on the feature branch, but do require manual conflict resolution. I think to get what you want (i.e., catch SemVer vioaltions ASAP) we would need to either start opening both PRs in parallel again rather than collecting all backports prior to release, or start opening the fixes first towards the backport branch, and then reopen and land them towards main once reviewed and merged.

[TheBlueMatt]

I don't think what you're describing is possible then as many of the backport-labeled PRs do not cleanly rebase on the feature branch, but do require manual conflict resolution.

Right, but this is CI, if it requires manual conflict resolution we can ignore the CI failure and make sure that its semver compatible by hand and automatically in the backport PR itself :).

[tnull]

Right, but this is CI, if it requires manual conflict resolution we can ignore the CI failure and make sure that its semver compatible by hand and automatically in the backport PR itself :).

Yes.

But now I'm confused what the point is you're making?: AFAIU you asked above whether it's possible to automatically cherry-pick check for SemVer-incompatible changes prior to opening the actual backport PR in CI?

And my TLDR is that, if we want to keep the current approach, it is not possible to do this automatically, but should still check in CI when opening the backport CI so that we can at least catch and remedy the issue then, even if it means going back to main afterwards. Do you agree?

[TheBlueMatt]

But now I'm confused what the point is you're making?: AFAIU you asked above whether it's possible to automatically cherry-pick check for SemVer-incompatible changes prior to opening the actual backport PR in CI?

My thinking is ideally we should (a) when a PR is opened with the backport X tag, we should run the semver check, (b) when the PR is merged (ideally CI will, but if not) we will open a (rollup) PR that includes all the backport PRs merged against the branch, (c) when that happens, this job gets run again and we check SemVer compat.

I believe (a) is possible using the hooks that were also used in @dunxen's PR.

[tnull]

My thinking is ideally we should (a) when a PR is opened with the backport X tag, we should run the semver check,

Alright, added a fixup dropping the ignore on main, which will us have run the checks on all PRs (which will succeed post-#3546).

Or are you saying to run the checks based on the 0.1 branch? Again, I'm not sure this is feasible in an automized way, even if we had some hooks for it? E.g., #3534 will need some additional manual conflict steps during rebasing as it uses internal APIs that have diverged on main since 0.1. I don't think we want/can force this to always happen automatically?

(b) when the PR is merged (ideally CI will, but if not) we will open a (rollup) PR that includes all the backport PRs merged against the branch,

Yup, we're doing this already.

(c) when that happens, this job gets run again and we check SemVer compat.

Yup, that's accomplished by on: push.

[TheBlueMatt]

Or are you saying to run the checks based on the 0.1 branch? Again, I'm not sure this is feasible in an automized way, even if we had some hooks for it? E.g., #3534 will need some additional manual conflict steps during rebasing as it uses internal APIs that have diverged on main since 0.1. I don't think we want/can force this to always happen automatically?

It won't always work, no, but in most cases it'll work. If it fails due to a rebase conflict, we can just ignore the CI job and validate it when we do backports, but its good to at least try.

[TheBlueMatt]

Can we filter it to just when the backport X tag is applied?

[tnull]

I assume you don't mean here but on: pull_request below? At least not generally, otherwise the PRs towards (eg. #3567) the release branches would also need that label, which doesn't make sense in our "needs to be backported" semantics.

We could potentially see whether it's possible to only apply it on PRs towards main that have the label but I don't really see the argument for the complication as running it on every PR shouldn't be that costly to begin with. But, if we really want to do it, I'd prefer to first land this PR and assert everything is robustly working as intended before introducing further complications, see below.

[TheBlueMatt]

I assume you don't mean here but on: pull_request below?

I meant by running via something like this:

on:
  pull_request_target:
    types: [opened, labeled]
...
  if: startsWith('backport', github.event.label.name)

otherwise the PRs towards (eg. #3567) the release branches would also need that label, which doesn't make sense in our "needs to be backported" semantics.

We can check for either condition, we can write code in if statements on the jobs :)

We could potentially see whether it's possible to only apply it on PRs towards main that have the label but I don't really see the argument for the complication as running it on every PR shouldn't be that costly to begin with.

Just annoying cause it'll always be failing, but if we filter on the label then it should only fail when its actually something worth investigating.

But, if we really want to do it, I'd prefer to first land this PR and assert everything is robustly working as intended before introducing further complications, see below.

Sure, that's fine.

[tnull]

Just annoying cause it'll always be failing, but if we filter on the label then it should only fail when its actually something worth investigating.

Why would it always be failing? It doesn't fail in either case, if the changes aren't breaking compared to the baseline. On main (where we bumped to 0.2.0+git) it picks 0.1 as the baseline, and in the feature branch the latest patch release.

[tnull]

It won't always work, no, but in most cases it'll work. If it fails due to a rebase conflict, we can just ignore the CI job and validate it when we do backports, but its good to at least try.

Hmm. I'm not sure I see the utility of adding another flaky CI check tbh. In any case, if we want to go there I'd prefer to do it in a follow-up to first assert that the current version is robust and working as intended.

[TheBlueMatt]

Hmm. I'm not sure I see the utility of adding another flaky CI check tbh. In any case, if we want to go there I'd prefer to do it in a follow-up to first assert that the current version is robust and working as intended.

Sure that's fine, if you prefer, feel free to drop the fixup commit and we can land this.

[tnull]

Sure that's fine, if you prefer, feel free to drop the fixup commit and we can land this.

I think there is a misconception that this job will always fail on PRs towards main. As mentioned above, it doesn't due to picking the right baseline in the right context.

E.g., when run on main:

main ~/workspace/rust-lightning/lightning> cargo semver-checks --default-features
    Building lightning v0.2.0+git (current)
       Built [   2.561s] (current)
     Parsing lightning v0.2.0+git (current)
      Parsed [   0.068s] (current)
     Parsing lightning v0.1.1 (baseline, cached)
      Parsed [   0.073s] (baseline)
    Checking lightning v0.1.1 -> v0.2.0+git (major change)
     Checked [   0.000s] 0 checks: 0 pass, 107 skip
     Summary no semver update required
    Finished [   2.975s] lightning

and when run on 0.1:

0.1 ~/workspace/rust-lightning/lightning> cargo semver-checks --default-features
    Building lightning v0.1.1 (current)
       Built [   6.831s] (current)
     Parsing lightning v0.1.1 (current)
      Parsed [   0.068s] (current)
     Parsing lightning v0.1.1 (baseline, cached)
      Parsed [   0.070s] (baseline)
    Checking lightning v0.1.1 -> v0.1.1 (no change)
     Checked [   0.119s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [   7.380s] lightning

[tnull]

Pushed a fixup with additional checks for crate features (let me know if I forgot something important). Undrafting.

Turns out I had forgotten to actually push the mentioned fixup commit. 🤦‍♂️

Now did so.

As for the other fixup: as mentioned above, I'd prefer to simply squash rather than drop it, as the CI check should just work fine on main/PRs towards main.

[tnull]

See https://github.com/lightningdevkit/rust-lightning/actions/runs/13112540715/job/36579407324 for an example run of the changes here.

[TheBlueMatt]

I think there is a misconception that this job will always fail on PRs towards main. As mentioned above, it doesn't due to picking the right baseline in the right context.

Ah, okay, hadn't realized it auto-skips on main. Happy to land this as-is if we look into the on-label workflow in the future.

[tnull]

Ah, okay, hadn't realized it auto-skips on main

It doesn't skip, but as we bumped to 0.2+git, it picks 0.1 as the baseline and detects that there has been a minor version bump.

Happy to land this as-is if we look into the on-label workflow in the future.

Sounds good, now squashed the fixups without further changes:

> git diff-tree -U2  9a0978bef 655d580ab
>

[TheBlueMatt]

It doesn't skip, but as we bumped to 0.2+git, it picks 0.1 as the baseline and detects that there has been a minor version bump.

Right, that's what I meant by "skip" :)