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

ABI compatibility checker #917

Draft
wants to merge 1 commit into
base: 0.27-maintenance
Choose a base branch
from
Draft

ABI compatibility checker #917

wants to merge 1 commit into from

Conversation

D4N
Copy link
Member

@D4N D4N commented Jun 18, 2019

This is a draft of a job on GitLab CI for the 0.27-maintenance branch that runs abi-compliance-checker to check the difference between the current state and the 0.27 tag. If there are notable differences, then the job fails. It furthermore uploads the report as an artifact for inspection.

Currently this doesn't work correctly yet, as the report doesn't contain any useful information at all. Maybe @cryptomilk can provide some insight to what I've missed?

@D4N D4N requested review from piponazo and clanmills June 18, 2019 23:38
@D4N D4N force-pushed the abi_compliance branch from a02f38e to 8469d79 Compare June 18, 2019 23:39
@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #917 into 0.27-maintenance will decrease coverage by 0.41%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           0.27-maintenance     #917      +/-   ##
====================================================
- Coverage             63.36%   62.94%   -0.42%     
====================================================
  Files                   159      157       -2     
  Lines                 23046    21618    -1428     
====================================================
- Hits                  14603    13608     -995     
+ Misses                 8443     8010     -433
Impacted Files Coverage Δ
include/exiv2/slice.hpp 47.5% <0%> (-44.66%) ⬇️
src/tags.cpp 56.68% <0%> (-22.43%) ⬇️
src/samsungmn_int.cpp 17.14% <0%> (-7.86%) ⬇️
src/olympusmn_int.cpp 19.23% <0%> (-6.81%) ⬇️
src/iptc.cpp 73.06% <0%> (-4.68%) ⬇️
src/image_int.cpp 40% <0%> (-4.24%) ⬇️
src/nikonmn_int.cpp 58.73% <0%> (-4.19%) ⬇️
src/crwimage_int.hpp 70% <0%> (-4.08%) ⬇️
samples/xmpsample.cpp 93.4% <0%> (-3.4%) ⬇️
unitTests/test_slice.cpp 96.92% <0%> (-3.08%) ⬇️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43a0747...380c14a. Read the comment docs.

piponazo
piponazo previously approved these changes Jun 19, 2019
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

That's a great addition to the project!

# abi-dumper complains if we don't pass -Og
CXXFLAGS: "-Og -g3 -gstrict-dwarf"
CFLAGS: "-Og -g3 -gstrict-dwarf"
# enabling curl is broken on 0.27
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it broken when consuming it from a system-package? from conan? both? Would you mind to create a ticket about this? I could check this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's broken when I checkout the tag directly, not from a system package and haven't tried conan.

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

I agree with @piponazo This is a wonderful addition to the CI machinery. Let's enable it on 0.27-maintenance once v0.27.2 has been released and we'll aim to retain that API/ABI for the rest of "the dots".

At the moment, there are 4000 external symbols (from about 26000 in total). I suspect there is an architecture to drastically reduce the API and enable us to add/remove/deprecate interfaces. For example COM (yes, it's ancient Microsoft Technology) uses QueryInterface(UUID) to say "Do you have this Interface". Almost everything in the library could be hidden (from an external point of view).

Perhaps we could introduce a new architecture with v0.28 as the prototype v1.0 API. I wonder how other libraries deal with this?

@D4N D4N force-pushed the abi_compliance branch from 8469d79 to 19eddf9 Compare July 3, 2019 19:15
@mergify mergify bot dismissed piponazo’s stale review July 3, 2019 19:16

Pull request has been modified.

@D4N
Copy link
Member Author

D4N commented Jul 3, 2019

Thanks to the amazing help provided by @lvc, I've been able to get this running on GitLab.

The solution to the problem was a bug in abi-dumper that caused issues with the pretty recent version of elfutils that is shipped by Fedora 30. I've worked around this issue by temporarily using the Fedora 29 container.

Let's see what the GitLab stage reports.

@D4N
Copy link
Member Author

D4N commented Jul 4, 2019

Nope, the issue is also present on Fedora 29, I'll have to update the package on Fedora and then this will start working (fingers crossed).

@D4N D4N force-pushed the abi_compliance branch 2 times, most recently from e6431bc to db730e2 Compare July 10, 2019 22:32
@D4N
Copy link
Member Author

D4N commented Jul 10, 2019

The abi compliance stage on GitLab succeded insofar as it now reports an error as 0.27 and 0.27-maintenance are not binary compatible.

So how should we approach this? Fix the ABI issues or just drop this?

@piponazo
Copy link
Collaborator

Great @D4N ! If the issue is easy to solve I would propose to fix it. We can create an issue about the specific issue, target it for the next dot release, and hopefully some of us could dedicate some time to it. If the issue is too complex to solve, then we can discuss more about what to do.

@D4N D4N force-pushed the abi_compliance branch 2 times, most recently from 6f136be to 69f4634 Compare July 13, 2019 08:30
@D4N D4N force-pushed the abi_compliance branch from 69f4634 to d593872 Compare August 2, 2019 20:38
@clanmills clanmills added this to the v1.00 milestone Apr 13, 2021
@clanmills clanmills mentioned this pull request Apr 14, 2021
@kevinbackhouse kevinbackhouse modified the milestones: v0.28.0, Backlog Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants