CI: Setup ABI testing - Githubissues

gujjwal00 commented 1 year ago

Fixes: #181

This PR adds a CI job to test ABI of client & server libraries. https://github.com/lvc/abi-dumper is used for dumping library ABI, and then https://github.com/lvc/abi-compliance-checker is used to check for any breakage. Both of these are available in Ubuntu repository.

Job Overview

vncclient & vncserver targets are built with debug info (required by abi-dumper to properly parse the library).
New ABI is checked against a reference ABI dump. Job will fail if ABIs are incompatible.
HTML reports generated by the checker, along with new ABI dumps, are uploaded as GitHub artifacts.
These reports can be consulted to know exactly what broke.

@bk138 I have included reference ABI dumps in utils/abi directory for now (just for testing). These are generated using this job itself. The two text files are ~950KB in total, and could be moved somewhere else.

I have temporarily disabled other CI jobs to reduce testing time. I will enable them when this PR is fully ready.

gujjwal00 commented 1 year ago

https://github.com/LibVNC/libvncserver/pull/551/commits/a86a0baa13c693ec99f49fc928ebfe2e7268b4f0 is a simple ABI break in rfbClient which should fail the CI build.

bk138 commented 1 year ago

Nice! I`ll have a look!

bk138 commented 1 year ago

Few thoughts:

There's some repetition in listing all the steps compared to the "normal" CI build. OTOH, checking ABI for every matrix combination also makes not that much sense. rfbconfig.h contents depend on what's detected on the system, not build settings AFAICR. What are your thoughts?
We can have the "current stable" ABI in the tree generated manually at release time or dynamically by looking at latest release tag and doing a dump of this before the current dump. While the latter seems less work in the long run, the former is an easier first step. What do you think? Edit: to answer my question, I think ABI breakage should be done intentionally, so very probably manually.
- Then the prepated dumps should be added to "test/abi".
- We should maybe have a helper script for generating the dumps? Or docs ;-)

gujjwal00 commented 1 year ago

Few thoughts:

* There's some repetition in listing all the steps compared to the "normal" CI build. OTOH, checking ABI for every matrix combination also makes not that much sense. rfbconfig.h contents depend on what's detected on the system, not build settings AFAICR. What are your thoughts?

I think abi-dumper only supports ELF binaries on Linux, so we are stuck here anyway. And as you said, if there is an ABI issue, it is likely to affect all platforms.

* We can have the "current stable" ABI in the tree generated manually at release time or dynamically by looking at latest release tag and doing a dump of this before the current dump. While the latter seems less work in the long run, the former is an easier first step. What do you think? Edit: to answer my question, I think ABI breakage should be done intentionally, so very probably manually.

You are right, we can't go fully dynamic with release tag. Otherwise, when we do want an ABI break, this job will continue to break until next release.

Fully manual approach seems have a drawback that we cannot let the reference ABI dump become stale. It has to be updated at-least for each release. Otherwise, we could end up in a situation where a field was added in n+1 version, then got shifted down in n+2 version due to another field, but comparing with version n ABI dump will not catch this break.

One solution is to test against a specific commit. This value could be updated whenever we want, e.g. at each release or after expected ABI break.

Also, do we have any explicit policy/plans regarding ABI breaks?

bk138 commented 1 year ago

Fully manual approach seems have a drawback that we cannot let the reference ABI dump become stale. It has to be updated at-least for each release. Otherwise, we could end up in a situation where a field was added in n+1 version, then got shifted down in n+2 version due to another field, but comparing with version n ABI dump will not catch this break.

One solution is to test against a specific commit. This value could be updated whenever we want, e.g. at each release or after expected ABI break.

Yeah that sounds reasonable. Basically for this PR please:

create a helper script in utils/ with maybe some comments that dumps current ABI to test/abi/. Or maybe have the script itself in test or test/abi, I actually dunno. Up to you :-) This is to be invoked by a human.
invoke it on the last release tag and commit the dump.
add some info about this to https://github.com/LibVNC/libvncserver/blob/master/README.md#cutting-a-release
test and tidy up the yaml

then we're done, or?

Also, do we have any explicit policy/plans regarding ABI breaks?

Yeah, after v1.0.0, see https://github.com/LibVNC/libvncserver/issues?q=is%3Aissue+is%3Aopen+label%3Arework-api

gujjwal00 commented 1 year ago

create a helper script in utils/ with maybe some comments that dumps current ABI to test/abi/. Or maybe have the script itself in test or test/abi, I actually dunno. Up to you :-) This is to be invoked by a human.

invoke it on the last release tag and commit the dump.

The problem here is how to make sure the build environment is consistent across different invocation of this script. ABI can be affected by available libraries, or CMake arguments. Any ABI dump we commit to repository will be valid only for that particular environment.

For example, I realized today that the CI job doesn't install any SASL library, so current ABI dump doesn't include SASL-related fields of rfbClient.

gujjwal00 commented 1 year ago

How about this:

Create a Python script check-abi.py in test/abi. Script takes following arguments:
- old-ref: reference to a git commit hash; optional; defaults to reading from test/abi/published-abi-ref
- new-ref: reference to a git commit hash; optional; defaults to current HEAD
For old-ref & new-ref each:
- Create a git worktree in temporary location
- Build library
- Dump ABI
Compare ABIs

Updating published ref could be as simple as git rev-list master --max-count=1 > test/abi/published-abi-ref.

gujjwal00 commented 1 year ago

Added initial draft of the script:

gaurav@electron:libvncserver $ ./test/abi/abi-check.py -h
usage: abi-check.py [-h] [-o OLD] [-n NEW] [-u]

Check ABI compatibility between two Git revisions

optional arguments:
  -h, --help  show this help message and exit
  -o OLD      Old revision; defaults to reading from 'published-abi-revision'
  -n NEW      New revision; defaults to 'HEAD'
  -u          Update 'published-abi-revision' file with current 'HEAD'

ghost commented 1 year ago

libvncclient is used in Apache Guacamole and there is an open issue(https://issues.apache.org/jira/browse/GUACAMOLE-1741) related to libvncclient v0.9.14. The issue is pushed to master and is expected to be rolled out in v0.9.15. This is the only thing pending in the milestone, just wanted to check the possible ETA for this. If someone can help me with the timeline it would be great. Thanks

bk138 commented 1 year ago

@abhijeetjhalm No real ETA, sorry. You'll be better off if you cherry-pick the needed change until the next release is ready.

LibVNC / libvncserver

CI: Setup ABI testing #551