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

CI: Setup ABI testing #551

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

gujjwal00
Copy link
Contributor

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
Copy link
Contributor Author

a86a0ba is a simple ABI break in rfbClient which should fail the CI build.

@bk138
Copy link
Member

bk138 commented Jan 24, 2023

Nice! I`ll have a look!

@bk138
Copy link
Member

bk138 commented Jan 24, 2023

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
Copy link
Contributor Author

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
Copy link
Member

bk138 commented Jan 25, 2023

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

@bk138 bk138 added this to the Release 0.9.15 milestone Jan 25, 2023
@bk138 bk138 self-assigned this Jan 25, 2023
@gujjwal00
Copy link
Contributor Author

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
Copy link
Contributor Author

gujjwal00 commented Jan 26, 2023

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 gujjwal00 force-pushed the ci-abi-test branch 3 times, most recently from a86a0ba to 8779729 Compare January 27, 2023 17:37
@gujjwal00
Copy link
Contributor Author

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
Copy link

ghost commented Mar 16, 2023

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
Copy link
Member

bk138 commented Mar 16, 2023

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

@bk138 bk138 removed their assignment Aug 5, 2023
@bk138 bk138 removed this from the Release 0.9.15 milestone Jul 2, 2024
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

Successfully merging this pull request may close these issues.

CI: add an abi check
2 participants