Add ABI checking tool for release process #1380

hebasto commented at 2:02 PM on July 18, 2023: member

Closes #1136.

An example of using the check-abi.sh tool against changes from #1140:

fanquake commented at 2:21 PM on July 18, 2023: member

Concept ~0. Looks like this tool is essentially unmaintained (and only currently works with workarounds (-gdwarf-4)). There have been requests for new releases / dwarf-5 support but they haven't gotten any response.

Are current/previous users migrating to something else?

hebasto commented at 2:25 PM on July 18, 2023: member

Looks like this tool is essentially unmaintained

Correct. I've noticed the same. But both packages still provided in Debian repositories. And this approach is still mentioned in https://sourceware.org/glibc/wiki/Testing/ABI_checker.

real-or-random added the label assurance on Jul 20, 2023

real-or-random added the label ci on Jul 20, 2023

in tools/check-abi.sh:12 in ec39a2baab outdated

   7 | +        -DCMAKE_C_COMPILER=gcc -DCMAKE_BUILD_TYPE=None -DCMAKE_C_FLAGS="-g -Og -gdwarf-4" \
   8 | +        -DSECP256K1_ENABLE_MODULE_ECDH=ON \
   9 | +        -DSECP256K1_ENABLE_MODULE_RECOVERY=ON \
  10 | +        -DSECP256K1_ENABLE_MODULE_EXTRAKEYS=ON \
  11 | +        -DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON \
  12 | +        -DSECP256K1_ENABLE_MODULE_ELLSWIFT=ON \

real-or-random commented at 9:56 PM on July 20, 2023:

I bet that we'll forget to update this as soon as we add the next module. I suggest using dev mode instead. Yes, it's intended for developers, but I think it's acceptable in this case. Or what do you think?

hebasto commented at 9:16 AM on July 21, 2023:

I bet that we'll forget to update this as soon as we add the next module. I suggest using dev mode instead.

We might forget to update CMakePresets.json as well :D

For example, it lacks SECP256K1_ENABLE_MODULE_ELLSWIFT: https://github.com/bitcoin-core/secp256k1/blob/c545fdc374964424683d9dac31a828adedabe860/CMakePresets.json#L8-L12

real-or-random commented at 11:18 AM on July 21, 2023:

ellswift is enabled by default, so we don't need to enable it here explicitly.

hebasto commented at 11:22 AM on July 21, 2023:

I mean, that "we'll forget to update this as soon as we add the next module" might be applied to the CMakePresets.json as well.

And using dev mode implies building examples, which is redundant for the ABI check.

real-or-random commented at 11:30 AM on July 21, 2023:

I mean, that "we'll forget to update this as soon as we add the next module" might be applied to the CMakePresets.json as well.

Sure, but that doesn't make the point invalid. I think fewer code locations that we can forget to update is still better, even if it's not zero.

And using dev mode implies building examples, which is redundant for the ABI check.

Ok, what I had in mind is that it could still be disabled manually. But even if we don't, it doesn't really hurt to build the examples.

hebasto commented at 12:04 PM on July 21, 2023:

I suggest using dev mode instead.

Done.

in doc/release-process.md:53 in ec39a2baab outdated

  45 | @@ -46,6 +46,13 @@ gcc -o ecdsa examples/ecdsa.c -I $dir/include -L $dir/lib*/ -l secp256k1 -Wl,-rp
  46 |     * if this is not a patch release
  47 |         * updates `_PKG_VERSION_*` and `_LIB_VERSION_*`  in `configure.ac` and
  48 |         * updates `project(libsecp256k1 VERSION ...)` and `${PROJECT_NAME}_LIB_VERSION_*` in `CMakeLists.txt`.
  49 | +
  50 | +ABI Compatibility must be checked with the [`check-abi.sh`](/tools/check-abi.sh) tool (installing packages `abi-dumper` and
  51 | + `abi-compliance-checker` might be required):
  52 | + ```shell
  53 | + tools/check-abi.sh $(git describe --abbrev=0) master

real-or-random commented at 9:57 PM on July 20, 2023:

On which platform(s) do we want to check?

hebasto commented at 9:20 AM on July 21, 2023:

Both packages are widely available among Linux distros, including Debian and Fedora:

I have no preferences regarding the platforms to use for the ABI check. Debian works.

real-or-random commented at 9:16 AM on December 7, 2023:

Worth adding --match "v*.*.*" or --first-parent to the git command?

real-or-random commented at 9:22 AM on December 7, 2023:

We could install it in the Docker image to make it available to all devs. Even if we don't use it on CI for now. Running it on CI and making the report available as an artifact would certainly be nice to have if you want to add it here, but it's more work and I don't think it's strictly necessary, so it could be added later (or never).

hebasto commented at 12:05 PM on December 16, 2023:

Thanks! Done.

real-or-random commented at 9:58 PM on July 20, 2023: contributor

Weak Concept ACK

Yeah, it's a bit sad to introduce an unmaintained tool. But this PR is just 44 LoCs, I think that's acceptable...

hebasto commented at 9:34 AM on July 21, 2023: member

My point is that checking the ABI compatibility with a dedicated tool is strictly better than an educated guess.

The suggested set of tools:

works fine
is used by other projects (for example, glibc)
is widely adopted in Linux distros repositories

Are current/previous users migrating to something else?

I've made searching twice, and did not find any alternatives users are migrating to. However, I did not ask people directly, for example glibc developers.

Looks like this tool is essentially unmaintained (and only currently works with workarounds (-gdwarf-4)). There have been requests for new releases / dwarf-5 support but they haven't gotten any response.

As this is a tool, not a dependency library, I think we can use it as long as it works regardless of its maintenance status. As far as I'm aware, there are no bugs in the tool that might affect our use case.

DWARF v4 is still well supported by the latest gcc. It is a legitimate option, not a "workaround".

fanquake commented at 9:52 AM on July 21, 2023: member

is used by other projects (for example, glibc)

The wiki you link too describes using ABI dumper on a version of glibc that is more than a decade old. Is it actually being used now? (I can't find any use in the glibc repos). If so, I guess they aren't maintaining those docs, as the ones listed in that wiki page wouldn't work.

It is a legitimate option, not a "workaround".

You are working around the fact that the tool doesn't support DWARF 5, which is the default for modern compilers.

hebasto commented at 9:54 AM on July 21, 2023: member

It is a legitimate option, not a "workaround".

You are working around the fact that the tool doesn't support DWARF 5, which is the default for modern compilers.

How can using DWARF v5 affect our use case?

real-or-random commented at 11:26 AM on July 21, 2023: contributor

The wiki page is indeed obsolete, so we should not count it as an argument for the tool.

But yeah, I also think -gdwarf-4 is not an issue. It's just not the default.

hebasto force-pushed on Jul 21, 2023

hebasto commented at 12:04 PM on July 21, 2023: member

@real-or-random's #1380 (review) has been addressed.

real-or-random commented at 3:13 PM on November 20, 2023: contributor

Concept ACK, I'll have another close look soon

in tools/check-abi.sh:1 in 9d4365cc07

real-or-random commented at 9:20 AM on December 7, 2023:

This should probably have a (short) usage description when called with no arguments.

Or another idea: Have defaults for $1 and $2. In particular, having a git describe default for $1 would make this easier to use without looking up docs etc. Then we could have a short description when called with --help or -h.

hebasto commented at 12:05 PM on December 16, 2023:

Thanks! Reworked.

real-or-random commented at 9:26 AM on December 7, 2023: contributor

Rebasing this will be good for testing. In the current branch, git describe says that the last tag is v0.3.2. :)

I'll add this to the upcoming milestone. If we want to have this, then it will be good to merge it before the release. @jonasnick @sipa It will be nice to have some Concept (N)ACKs here.

real-or-random added this to the milestone 0.4.1 on Dec 7, 2023

real-or-random removed the label ci on Dec 7, 2023

real-or-random added the label release on Dec 7, 2023

in doc/release-process.md:50 in 9d4365cc07 outdated

  45 | @@ -46,6 +46,13 @@ gcc -o ecdsa examples/ecdsa.c -I $dir/include -L $dir/lib*/ -l secp256k1 -Wl,-rp
  46 |     * if this is not a patch release
  47 |         * updates `_PKG_VERSION_*` and `_LIB_VERSION_*`  in `configure.ac` and
  48 |         * updates `project(libsecp256k1 VERSION ...)` and `${PROJECT_NAME}_LIB_VERSION_*` in `CMakeLists.txt`.
  49 | +
  50 | +ABI Compatibility must be checked with the [`check-abi.sh`](/tools/check-abi.sh) tool (installing packages `abi-dumper` and

jonasnick commented at 8:00 PM on December 7, 2023:

Why is the ABI compatibility checked between step 1 and 2? Shouldn't this paragraph be part of the enumeration as well (and get number 2)?

But actually shouldn't this be done before updating the version number and writing the release notes?

hebasto commented at 1:21 PM on December 14, 2023:

Thanks! Reworked.

jonasnick commented at 8:05 PM on December 7, 2023: contributor

Concept ACK. The tool & check-abi.sh appear to work as intended. abi-dumper and abi-compliance-checker are available on NixOS. I think this can be a helpful addition to the release process at the cost of adding a small shell script.

hebasto force-pushed on Dec 14, 2023

hebasto commented at 1:21 PM on December 14, 2023: member

Rebasing this will be good for testing. In the current branch, git describe says that the last tag is v0.3.2. :)

Rebased. @jonasnick's comment has been addressed.

hebasto force-pushed on Dec 16, 2023

hebasto commented at 12:04 PM on December 16, 2023: member

@real-or-random's comments have been addressed. I apologize for any delay.

in doc/release-process.md:41 in 6bed465d96 outdated

  36 | @@ -37,7 +37,11 @@ gcc -o ecdsa examples/ecdsa.c -I $dir/include -L $dir/lib*/ -l secp256k1 -Wl,-rp
  37 |  
  38 |  ## Regular release
  39 |  
  40 | -1. Open a PR to the master branch with a commit (using message `"release: prepare for $MAJOR.$MINOR.$PATCH"`, for example) that
  41 | +1. Check the ABI compatibility with the [`check-abi.sh`](/tools/check-abi.sh) tool (installing the `abi-dumper` and `abi-compliance-checker` packages might be required):
  42 | +```shell
  43 | +tools/check-abi.sh
  44 | +```

jonasnick commented at 9:44 AM on December 18, 2023:

nit: how about we move this to the sanity checks section since it's also relevant for maintenance releases:

4. Use the [`check-abi.sh`](/tools/check-abi.sh) tool to ensure there are no unexpected ABI incompatibilities and that the version number and release notes accurately reflect all potential ABI changes. To run this tool, the `abi-dumper` and `abi-compliance-checker` packages are required.

\```shell
tools/check-abi.sh
\```

hebasto commented at 12:43 PM on December 18, 2023:

Thanks! Done.

jonasnick commented at 9:45 AM on December 18, 2023: contributor

Can confirm that the tool works according to the help output and interprets the arguments correctly.

hebasto force-pushed on Dec 18, 2023

real-or-random approved

real-or-random commented at 4:14 PM on December 20, 2023: contributor

ACK 09f9472879277d3c335e7a00741b407c4bc35dcf Tested and works nicely :)

I'm happy to reACK quickly this with my suggestion applied, but I'm also happy to get this merged as-is for the release

hebasto commented at 4:22 PM on December 20, 2023: member

I'm happy to reACK quickly this with my suggestion applied...

Do you mean this one?

in tools/check-abi.sh:62 in 09f9472879 outdated

  57 | +cd "$current_build_dir"
  58 | +configure_and_build "$source_dir"
  59 | +abi-dumper src/libsecp256k1.so -o ABI.dump -lver "$new_version"
  60 | +cd "$source_dir"
  61 | +
  62 | +abi-compliance-checker -lib libsecp256k1 -old "${base_build_dir}/ABI.dump" -new "${current_build_dir}/ABI.dump"

real-or-random commented at 4:34 PM on December 20, 2023:

Here's a change that uses also temporary source trees (instead of just build trees):

checkout_and_build() {
    git worktree add -d "$1"
    cd "$1"
    mkdir build && cd build
    cmake -S .. --preset dev-mode \
        -DCMAKE_C_COMPILER=gcc -DCMAKE_BUILD_TYPE=None -DCMAKE_C_FLAGS="-g -Og -gdwarf-4" \
        -DSECP256K1_BUILD_BENCHMARK=OFF \
        -DSECP256K1_BUILD_TESTS=OFF \
        -DSECP256K1_BUILD_EXHAUSTIVE_TESTS=OFF \
        -DSECP256K1_BUILD_CTIME_TESTS=OFF \
        -DSECP256K1_BUILD_EXAMPLES=OFF
    cmake --build . -j "$(nproc)"
}

echo "Comparing $base_version (base version) to $new_version (new version)"
echo

base_source_dir=$(mktemp -d)
checkout_and_build "$base_source_dir"
abi-dumper src/libsecp256k1.so -o ABI.dump -lver "$base_version"

current_source_dir=$(mktemp -d)
checkout_and_build "$current_source_dir"
abi-dumper src/libsecp256k1.so -o ABI.dump -lver "$new_version"

abi-compliance-checker -lib libsecp256k1 -old "${base_source_dir}/build/ABI.dump" -new "${current_source_dir}/build/ABI.dump"

real-or-random commented at 4:36 PM on December 20, 2023: contributor

Do you mean this one?

Sorry, I meant the one I've just posted now... :D (Sorry, not sure if this got lost in transit, or if I just forgot to click the "Comment" button...)

hebasto force-pushed on Dec 20, 2023

hebasto commented at 4:45 PM on December 20, 2023: member

Do you mean this one?

Sorry, I meant the one I've just posted now... :D (Sorry, not sure if this got lost in transit, or if I just forgot to click the "Comment" button...)

Thanks! Done.

hebasto commented at 4:51 PM on December 20, 2023: member

Do you mean this one?

Sorry, I meant the one I've just posted now... :D (Sorry, not sure if this got lost in transit, or if I just forgot to click the "Comment" button...)

Sorry. The new script builds the same commit twice.

real-or-random commented at 4:58 PM on December 20, 2023: contributor

Argh, sorry -.-

I think we'll need to add the git revision as an arg to git worktree add -d "$1", like git worktree add -d "$1" <tag>.. And then move the git worktree command out of the function again... I can look into it later, but I don't have the time right now.

hebasto force-pushed on Dec 20, 2023

Add `tools/check-abi.sh`

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

e7f830e32c

doc: Add ABI checking with `check-abi.sh` to the Release Process 74a4d974d5

hebasto force-pushed on Dec 20, 2023

hebasto commented at 5:41 PM on December 20, 2023: member

Fixed minor issues in the recent implementation.

Suggesting to test by running tools/check-abi.sh v0.3.2 v0.4.0. The resulted report must be the same as in #1415 (comment).

real-or-random approved

real-or-random commented at 9:01 PM on December 20, 2023: contributor

ACK 74a4d974d5c81fbc437287dffc453028509682ab it compares the right commits now

jonasnick commented at 9:12 PM on December 20, 2023: contributor

re-Concept ACK 74a4d974d5c81fbc437287dffc453028509682ab

The tool continues to work for me.

jonasnick merged this on Dec 20, 2023

jonasnick closed this on Dec 20, 2023

hebasto deleted the branch on Dec 21, 2023