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

Conversation

@t-b
Copy link
Collaborator

@t-b t-b commented Aug 27, 2020

So here is my stab at fixing the ABI checking.

This PR includes a commit which breaks the ABI, just to see that it is working better now. Judging from the open issues on the involved projects I doubt that this is foolproof, but at least I could get it working now. If we decide that this approach is too kludgy we can also just remove it again.

I'm getting the following now when I include the breaking commit

--- old.dump    2020-08-27 21:25:02.180190259 +0200
+++ new.dump    2020-08-27 21:25:02.296190257 +0200
@@ -6743,7 +6743,7 @@
                                             'Class' => '1040135',
                                             'Header' => undef,
                                             'Line' => '115',
-                                            'MnglName' => '_ZN5Tango7ApiUtil18get_asynch_repliesEl',
+                                            'MnglName' => '_ZN5Tango7ApiUtil18get_asynch_repliesEi',
                                             'Param' => {
                                                          '0' => {
                                                                   'name' => 'this',
@@ -6751,7 +6751,7 @@
                                                                 },
                                                          '1' => {
                                                                   'name' => 'call_timeout',
-                                                                  'type' => '18757'
+                                                                  'type' => '18744'
                                                                 }
                                                        },
                                             'Return' => '1',
@@ -237394,7 +237394,7 @@
                                                   '_ZN5Tango7ApiUtil14device_to_attrERKNS_15DeviceAttributeERNS_16AttributeValue_4E' => 1,                                                                                                                                      
                                                   '_ZN5Tango7ApiUtil14get_ip_from_ifERSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaIS7_EE' => 1,                                                                                                             
                                                   '_ZN5Tango7ApiUtil15set_sig_handlerEv' => 1,
-                                                  '_ZN5Tango7ApiUtil18get_asynch_repliesEl' => 1,
+                                                  '_ZN5Tango7ApiUtil18get_asynch_repliesEi' => 1,
                                                   '_ZN5Tango7ApiUtil18get_asynch_repliesEv' => 1,
                                                   '_ZN5Tango7ApiUtil19attr_to_device_baseINS_16AttributeValue_4EEEvPKT_PNS_15DeviceAttributeE' => 1,                                                                                                                            
                                                   '_ZN5Tango7ApiUtil19attr_to_device_baseINS_16AttributeValue_5EEEvPKT_PNS_15DeviceAttributeE' => 1,                                                                                                                            
ABI/API breakages detected!

and no complaint when I revert the ABI breaking commit.

This approach does give quite some false positives, but I don't see how I can get the original approach working.

@t-b t-b requested review from bourtemb and mliszcz as code owners August 27, 2020 19:40
@t-b t-b force-pushed the bugfix/fix-abi-checking branch from 874b14e to 51dc328 Compare August 27, 2020 20:09
@t-b
Copy link
Collaborator Author

t-b commented Aug 27, 2020

@coveralls
Copy link

coveralls commented Aug 27, 2020

Coverage Status

Coverage increased (+0.6%) to 48.978% when pulling df2a9b5 on bugfix/fix-abi-checking into 1945e65 on tango-9-lts.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

t-b added 2 commits November 1, 2020 21:35
Turns out that the compliance check tool does not work in the current
setup.  The issue tracker [1] also suggests that we are not the only
one.

After fiddling with a lot of different flags and options of the
abi-dumper and the abi-compliance-checker tool, it turns out that diff
can help us here. We just diff the textual representation of the dump,
and if we have differences, it most likely broke.

The sed invocation removes the volatile parts of the dump with a fixed
string.

This also makes getting the html report as artifact obsolete.

[1]: https://github.com/lvc/abi-compliance-checker/issues
This allows for convenient rebuilds on developer machines which are not
pristine.
@t-b t-b force-pushed the bugfix/fix-abi-checking branch from 51dc328 to df2a9b5 Compare November 1, 2020 20:36
@t-b t-b added high priority Ready For Merge Candidate For Backport Requires a backport to the release branches labels Nov 1, 2020
@t-b
Copy link
Collaborator Author

t-b commented Nov 1, 2020

@mliszcz @bourtemb So here is the fixed version now.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mliszcz
Copy link
Collaborator

mliszcz commented Nov 2, 2020

@t-b have you tried using abi-dumper -compare OLD.dump NEW.dump instead of plain diff? Maybe it will be a bit more robust (if if works). https://manpages.debian.org/unstable/abi-dumper/abi-dumper.1.en.html

Edit: Actually these dumps look like serialized perl data structures and the dumper tool seems to treat them as such: https://github.com/lvc/abi-dumper/blob/master/abi-dumper.pl#L6578-L6634
So if abi-dumper -compare won't work we may try to write a perl one-liner that would load both dumps, pass them into https://metacpan.org/pod/Data::Diff and print the result?

@bourtemb
Copy link
Member

bourtemb commented Dec 4, 2020

@t-b have you tried using abi-dumper -compare OLD.dump NEW.dump instead of plain diff? Maybe it will be a bit more robust (if if works). https://manpages.debian.org/unstable/abi-dumper/abi-dumper.1.en.html

@t-b I think you didn't reply to this question from @mliszcz

@t-b
Copy link
Collaborator Author

t-b commented Dec 4, 2020

@bourtemb THanks for the reminder. I will have a look, altough I think that we could also just use what we have and see if it works in practice.

@t-b
Copy link
Collaborator Author

t-b commented Jan 19, 2021

@mliszcz @bourtemb
My earlier version used abi-dumper but as I said in the commit message, this had way too many bugs (false positives). How about we merge that now, given it does work, and later on improve it if required.

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, fine with me! This is still better than what we already have.

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.

@t-b thanks for this PR.
We'll see in practice how it behaves.
I just added a small suggestion to remove a useless (I think, unless this option is useful on some operating system versions?) option in the diff but this option should not cause any harm in any case so it can be merged like that too.

-list-affected || print_abi_api_breakages
sed "s/${old_revision}/fixed/g" ${base}/libtango-old-${old_revision}.dump > old.dump
sed "s/${new_revision}/fixed/g" ${base}/libtango-new-${new_revision}.dump > new.dump
diff -Nur old.dump new.dump || exit_on_abi_api_breakages
Copy link
Member

Choose a reason for hiding this comment

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

diff -r option (to recursively compare subdirectories) does not hurt but is not needed here.

-list-affected || print_abi_api_breakages
sed "s/${old_revision}/fixed/g" ${base}/libtango-old-${old_revision}.dump > old.dump
sed "s/${new_revision}/fixed/g" ${base}/libtango-new-${new_revision}.dump > new.dump
diff -Nur old.dump new.dump || exit_on_abi_api_breakages
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
diff -Nur old.dump new.dump || exit_on_abi_api_breakages
diff -Nu old.dump new.dump || exit_on_abi_api_breakages

@t-b
Copy link
Collaborator Author

t-b commented Jan 28, 2021

@bourtemb Thanks for the review. Yes the 'r' to diff is not needed, I would still merge as is, as it does not hurt and we can finally get this merged.

@t-b t-b merged commit f8b4e0b into tango-9-lts Jan 28, 2021
@t-b t-b deleted the bugfix/fix-abi-checking branch January 28, 2021 13:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Candidate For Backport Requires a backport to the release branches high priority Ready For Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants