Skip to content
This repository has been archived by the owner on Jul 8, 2022. It is now read-only.

Automate abi api compliance check #662

Merged
merged 5 commits into from
Jun 15, 2020
Merged

Automate abi api compliance check #662

merged 5 commits into from
Jun 15, 2020

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Jan 22, 2020 •
edited
Loading

@t-b t-b force-pushed the automate-abi-api-compliance-check branch 2 times, most recently from 83dd536 to adde45d Compare January 23, 2020 21:02
@t-b t-b marked this pull request as ready for review January 23, 2020 21:15
@t-b t-b requested review from bourtemb and mliszcz as code owners January 23, 2020 21:15
@t-b t-b force-pushed the automate-abi-api-compliance-check branch 2 times, most recently from 1235ccb to 887243c Compare January 23, 2020 23:06
@t-b
Copy link
Collaborator Author

t-b commented Jan 24, 2020

The test error is unrelated, the output looks like

Creating compatibility report ...

Binary compatibility: 100%

Source compatibility: 100%

Total binary compatibility problems: 0, warnings: 0

Total source compatibility problems: 0, warnings: 0

Report: compat_reports/libtango/85fbfd3aebddb629e7fcb8e15106c4add89e42b6_to_29fbad5248830849a04062bcdbc91ba99c18d397/compat_report.html

I've not yet found a good place for this kind of artefact. Ideas?

@bourtemb
Copy link
Member

I've not yet found a good place for this kind of artifact. Ideas?

@t-b, as suggested by @mliszcz for another problem, the best would probably be to use Github actions for this kind of job.

Github actions support artifacts.

@t-b t-b force-pushed the automate-abi-api-compliance-check branch from 887243c to 821ef6d Compare February 17, 2020 20:01
@t-b t-b removed the Has Conflicts label Feb 17, 2020
@t-b t-b mentioned this pull request Feb 27, 2020
Merged
7 tasks
@t-b t-b added Candidate For Backport Requires a backport to the release branches Ready For Merge and removed Candidate For Backport Requires a backport to the release branches Ready For Merge labels Feb 27, 2020
@t-b t-b self-assigned this Apr 9, 2020
@t-b t-b force-pushed the automate-abi-api-compliance-check branch from 821ef6d to fd73c0d Compare April 23, 2020 14:43
@t-b
Copy link
Collaborator Author

t-b commented Apr 23, 2020

Rebased. Regarding the artefacts. Do we really want to move to github actions just because of the artifacts? According to 1 they only store artifacts for 90 days for PRs ("For pushes and pull requests, GitHub stores artifacts for 90 days.") and I don't think this is enough in general.

Bintray would be the obvious alternative. Otherwise I can probably also come up with free S3 storage.

@bourtemb
Copy link
Member

Rebased. Regarding the artefacts. Do we really want to move to github actions just because of the artifacts? According to 1 they only store artifacts for 90 days for PRs ("For pushes and pull requests, GitHub stores artifacts for 90 days.") and I don't think this is enough in general.

Bintray would be the obvious alternative. Otherwise I can probably also come up with free S3 storage.

I don't think we need to keep the ABI compatibility reports for ages so for the use case we were talking in this issue it should be fine.
I think we will have a look at the report only when an incompatibility will have been reported. So hopefully 90 days should be enough and if not, I think we can re-run the github action or equivalent to generate it again?

@t-b
Copy link
Collaborator Author

t-b commented Apr 23, 2020

@bourtemb I'll give github actions a try for that.

mliszcz
mliszcz reviewed May 4, 2020
View reviewed changes
Copy link
Collaborator

@mliszcz mliszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @t-b. I understand this is still work in progress, but I added some comments for you to consider that may simplify this PR a bit.

configure/CMakeLists.txt Outdated Show resolved Hide resolved
configure/CMakeLists.txt Outdated Show resolved Hide resolved
.travis/install_tango.sh Outdated Show resolved Hide resolved
.travis/check-ABI-API-compliance.sh Outdated Show resolved Hide resolved
@t-b t-b force-pushed the automate-abi-api-compliance-check branch from 3f7e839 to 64504f0 Compare June 11, 2020 09:44
@t-b t-b mentioned this pull request Jun 11, 2020
Closed
@t-b
configure/CMakeLists.txt: Enhance and simplify compile flags on *nix
d46dd5a 
All supported GCC/Clang versions

GCC: supports '-Og -g' since 4.8 (Debian 8 - oldest OS we support in CI has GCC 4.9)
Clang: supports '-Og -g' since 4.0.0

support '-g' so we don't need to check for that. Additionally we want to
compile with '-Og' for abi compliance-checks [1].

And we also add the relevant security compile options from
dpkg-buildflags. -fstack-protector strong is only added for the release
builds as that might interfere for debug builds.

[1]: https://lvc.github.io/abi-compliance-checker/
@t-b t-b force-pushed the automate-abi-api-compliance-check branch 7 times, most recently from 1e8d10a to 803d019 Compare June 11, 2020 15:57
t-b added 2 commits June 11, 2020 18:22
@t-b
Add ABI/API compliance checking using github actions
ca14580 
Maintaining ABI/API compatibility in C/C++ is a difficult task.
Fortunately the abi-compliance-checker tool [1] can help examining the
current state and compare different versions.

The script check-ABI-API-compliance.sh allows to compare the current
revision with the revision from the reference branch (currently
tango-9-lts).

This script is now executed as github actions job, but can also be executed
locally. We choose github actions because that allows to store artifacts
for 90 days for each PR.

[1]: https://lvc.github.io/abi-compliance-checker/
@t-b
.github/workflows/c-cpp.yml: Allow ABI/API compliance check to fail
30d3245 
We don't care usually too much about for PRs targeting tango-9-lts.
@t-b t-b requested a review from mliszcz June 11, 2020 16:24
@t-b t-b force-pushed the automate-abi-api-compliance-check branch from d626a5b to 30d3245 Compare June 11, 2020 16:24
@t-b
Copy link
Collaborator Author

t-b commented Jun 11, 2020

@bourtemb @mliszcz Ready.

@t-b t-b removed their assignment Jun 11, 2020
@t-b t-b added Ready For Merge Candidate For Backport Requires a backport to the release branches labels Jun 11, 2020
bourtemb
bourtemb approved these changes Jun 12, 2020
View reviewed changes
Copy link
Member

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @t-b
Looks good to me.

Do you know whether these errors reported in the log are important?:

Reading debug-info
WARNING: a "Struct" type with no attributes detected in the DWARF dump (19012805)
Reading v-tables
ERROR: missed type name (241)
ERROR: missed type name (1881813)
ERROR: missed type name (751092)
ERROR: missed type name (751101)
ERROR: missed type name (751162)
ERROR: missed type name (751171)
ERROR: missed type name (1879520)

@t-b
Copy link
Collaborator Author

t-b commented Jun 12, 2020

@bourtemb These are from the ABI dumper tool. This could be lvc/abi-dumper#21. I'll have a look.

@t-b
Copy link
Collaborator Author

t-b commented Jun 12, 2020 •
edited
Loading

@bourtemb I've tried the hint from the issue passing -lambda but the warnings don't dissapear. I would leave it like it is.

mliszcz
mliszcz approved these changes Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@mliszcz mliszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good. I just replied to your comment regarding refs which we are comparing to detect ABI breakage. I do know which is the preferred way so I wouldn't mind if you just drop that comment. Thanks!

@bourtemb
Copy link
Member

@bourtemb I've tried the hint from the issue passing -lambda but the warnings don't dissapear. I would leave it like it is.

@t-b did you try the other hint. Use the latest version from the master branch of abi-dumper tool?
I don't like much this approach because if they introduce a bug in their master branch, we get it.
Their latest release was done in 2017 and it looks like there a a few changes since then (11 commits compared to their latest release), including some warning fixes.

@t-b
Copy link
Collaborator Author

t-b commented Jun 15, 2020

@bourtemb No I did not try latest master of abi-dumper. Feel free to create an issue and assign me if I should give it a try.

I'll merge that now.

@t-b t-b merged commit 05b9558 into tango-controls:tango-9-lts Jun 15, 2020
@t-b t-b deleted the automate-abi-api-compliance-check branch June 15, 2020 20:40
@t-b t-b mentioned this pull request Jun 25, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bourtemb bourtemb bourtemb approved these changes

@mliszcz mliszcz mliszcz approved these changes

Assignees
No one assigned
Labels
Candidate For Backport Requires a backport to the release branches Ready For Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automate abi compliance check
3 participants
@t-b @bourtemb @mliszcz