Recently, gating was turned on for Fedora updates. abicheck was one of the checks chosen to be gating.
abicheck
Since the gating was turned on, we've had several reports of cases where the test is failing which the package maintainer / upstream author don't consider to really be valid. One example is an update to gap-pkg-io, reported by the maintainer, @jjames . @puiterwijk (IIRC) also mentioned a case to me in-person. And I think it's pretty clear that there will be many other cases.
From the results we've examined - I haven't looked in detail into the code - it seems like the task considers all or most DSOs (dynamic shared objects) present in any given package. However, many packages ship DSOs which aren't really intended for external consumption; e.g. plugins can often be shipped as DSOs, but generally aren't meant for other code to link against. In the gap-pkg-io case, @jjames describes the situation as "It comes with a bunch of addon packages that provide specific mathematical capabilities. Most of them are written solely in the scripting language, but a few have to interface with external libraries. When that is required, these packages build a small internal-only shared object to act as the glue between the external library and the scripting language."
So, I think we need the task to do more filtering to only fail for changes in DSOs that we're pretty confident are genuinely 'shared libraries'. I think it'd be better to err on the side of not checking all DSOs rather than checking too many, at least at first. To this end, my test balloon suggestion is:
Only changes in DSOs installed to /usr/lib or /usr/lib64 - NOT including subdirectories - should be considered failures.
/usr/lib
/usr/lib64
This is clearly not checking everything: just off the top of my head, there's the possibility of installing a config snippet in /etc/ld.so.conf.d/ that adds another directory to the ldconfig path, thus making DSOs in that directory public. We certainly have packages that do this. But I suggest we should start from a minimal point and move carefully from there to broader coverage, rather than start from covering too much and then try to narrow down.
/etc/ld.so.conf.d/
Note that this task has now been taking off the list of gating tests and I believe won't be added back until this issue is resolved somehow.
Please do add your thoughts on how we should deal with this :) For now, tagging @ralph , @tflink , @jskladan , @kparal , @sinnykumari , @pingou , @mattdm .
@dodji do you want to chime in on this one?
Even that is not sufficient to cover all cases:
-private-devel
I don't think dist.abicheck can be repaired into something that is reasonable to automatically enforce at all.
dist.abicheck
If we want to proceed with generally requiring abicheck, we could add Qt to a permanent blacklist of packages for which the check is not required. The usefulness of this approach depends on how big that list would be.
Users can define what they want to see filtered out by adding a suppression specification file to their package. The suppression specification file needs to end with the .abignore file extension.
We could start with a content of suppression specification file that would filter out files that are not in /usr/ib and /usr/lib64/. It would look like:
[suppress_file] # Suppress reports about files that are not in /usr/lib and /usr/lib64 filename_not_regexp = (/usr/lib/.*|/usr/lib64/.*)
The syntax of suppression specification files is described at https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppression-specifications.
If all (or most of packages) need to define the same suppression specifications then we can add those the the set of default suppression specifications applied by Libabigail no matter what. Just so you have an idea, the current default suppression specification file that is used by Libabigail is: https://sourceware.org/git/gitweb.cgi?p=libabigail.git;a=blob;f=default.abignore.
So I guess we need to work together to come up with the default suppression specification file that would be better suited for everyone.
Indeed, it's a question how much this should be done upstream and how much downstream. I was kinda thinking that this is really a downstream thing, as it's not necessarily the case that no-one would ever want to use libabigail to look for interface changes in a non-public DSO like this: for instance, an app maintainer might want to know about changes in plugin DSOs. So, I'm not sure that implementing this entirely upstream is the right way to go. Just my thought, though!
@ralph the reason I'm intending to look through the last X abicheck failures is basically to get some data on the question of just how many of these cases we have, yeah - it's kind of unknown at present. I was intentionally vague in the description of this bug in terms of exactly how both upstream libabigail and the downstream task behave in terms of choosing what to compare and what to ignore, because I didn't know for sure. I know it's not quite as simple as 'every DSO gets checked', but I'm not entirely sure what filters what out, and how that winds up working in practical terms when we look at the Fedora package set.
If some cases are specific to a given package, then the maintainer can add its own suppression specification file to the package and task-abicheck/libabigail will pick it up.
I think this is quite a strong assertion. Why can't you just come up with your own suppression specification file?
Adam Williamson pagure@pagure.io a =C3=A9crit:
adamwill added a new comment to an issue you are following: `` Indeed, it's a question how much this should be done upstream and how much downstream. I was kinda thinking that this is really a downstream thing, as it's not necessarily the case that no-one would ever want to use libabigail to look for interface changes in a non-public DSO like this: for instance, an app maintainer might want to know about changes in plugin DSOs, for instance. So, I'm not sure that implementing this entirely upstream is the right way to go. Just my thought, though!
Ah, it's true that task-abicheck could also define its own suppression specification file and pass that to Libabigail, rather than having Libabiga= il update its default one. The suppression specification mechanism should be flexible enough for task-abicheck to define the policy it wants.
--=20 Dodji
This makes the most sense to me - have a way for packages to say "ignore this".
@tflink I think the idea is we can use the suppression files to do two things: we can define a global policy at the level of the task with a suppression file there, and individual packages could ship their own suppression file if they hit false failures even with the task-level suppression file (e.g. the Qt case Kevin is describing). @dodji , do suppression files combine or supersede, or what?
Tim Flink pagure@pagure.io a =C3=A9crit:
tflink added a new comment to an issue you are following: `` Ah, it's true that task-abicheck could also define its own suppression specification file and pass that to Libabigail, rather than having Libab= iga=3D il update its default one. The suppression specification mechanism should = be flexible enough for task-abicheck to define the policy it wants. This makes the most sense to me - have a way for packages to say "ignore this".
tflink added a new comment to an issue you are following: ``
Ah, it's true that task-abicheck could also define its own suppression specification file and pass that to Libabigail, rather than having Libab= iga=3D il update its default one. The suppression specification mechanism should = be flexible enough for task-abicheck to define the policy it wants.
Actually, every single package maintainer can add a .abignore (a suppression specification) file to her package to say "ignore this". Libaibgail will pick that up.
What I was talking about earlier (that you replied to) is not that. It was more the task-abicheck tool that defines a policy that is global to all packages, without forcing upstream libabigail to do so.
I hope this helps.
adamwill added a new comment to an issue you are following: `` @tflink I think the idea is we can use the suppression files to do two things: we can define a global policy at the level of the task with a suppression file there, and individual packages could ship their own suppression file if they hit false failures even with the task-level suppression file (e.g. the Qt case Kevin is describing).
You are totally right, Adam.
@dodji , do suppression files combine or supersede, or what?
They combine.
@dodji, could you please add instructions about adding suppression files into individual packages to the dist.abicheck documentation here? https://fedoraproject.org/wiki/Taskotron/Tasks/dist.abicheck I'll then add some info about the global suppression file we'll distribute with task-abicheck, once we have it ready. Thanks a lot.
Also, is the suppression applied before or after execution? For some packages the abipkgdiff runs for a very long time and I hope this will help us improve the performance.
Kamil P=C3=A1ral pagure@pagure.io a =C3=A9crit:
kparal added a new comment to an issue you are following: `` @dodji, could you please add instructions about adding suppression files into individual packages to the dist.abicheck documentation here? https://fedoraproject.org/wiki/Taskotron/Tasks/dist.abicheck
I have started to document that at https://fedoraproject.org/wiki/How_to_filter_libabigail_reports.
I'll link that page from https://fedoraproject.org/wiki/Taskotron/Tasks/dist.abicheck as you mentionned above, once I am done with it.
I'll then add some info about the global suppression file we'll distribut= e with task-abicheck, once we have it ready. Thanks a lot.
Right.
Also, is the suppression applied before or after execution?
It depends. Some suppression specification directives are applied before the ABI comparison takes place. For instance, suppressing an entire file makes it so that the file is not even compared.
For some packages the abipkgdiff runs for a very long time and I hope this will help us improve the performance.
Yes, that can help with performance, indeed.
@kparal:
I have added the suppression specification documentation to https://fedoraproject.org/wiki/Taskotron/Tasks/dist.abicheck.
Feel free to check it out and tell me what you think!
Thanks!
Metadata Update from @kparal: - Issue assigned to kparal
I talked to @dodji and we figured out that currently the supressions can only be applied on a base file name, but not on file path. For that, a new config property is required (@dodji will be working on a patch).
However, @dodji has some additional ideas how to automatically recognize public .so from a private .so, I'll let him write the proposal here so that everyone can publish their thoughts on it.
Indeed. There is a tracking enhancement request for the the new filtering property that would apply to a path at https://sourceware.org/bugzilla/show_bug.cgi?id=22798 and I'll be working on that ASAP. Sorry for the inconvenience.
Also, it seems like there are cases where we could probably automatically guess if a given DSO is private. For instance, if the DSO is NOT in the 'Provides' of the RPM. In that case, abipkgdiff (the libabigail tool used by task-abicheck) could just ignore the DSO.
Would this fly? If yes, I'll implement that scheme.
Off the top of my head, I think that would at least be an improvement, yes.
edit: to provide a bit more detail on this: Fedora package library provides depend very heavily on the RPM auto-provides code, which is magic. Well, OK, it basically gets implemented by rpmdeps, calling stuff from rpmfc - you have to grok that lot to grok what it does. I guess we can say with relative confidence that it must be generating sufficient shared object Provides / Requires, cos otherwise we'd have lots of 'required library not being installed' bugs, and we don't. But I'm not sure if anyone knows for sure whether it generates too many - it could possibly be generating provides that aren't needed or used and we wouldn't necessarily know about that. But, we can try it and see.
Another case where we can avoid doing ABI comparison on a DSO, when - No header file is shipped in any of sub-packages (of course after excluding debuginfo and src rpms). No public header can mean that package is not meant for consumption by other packages. I am not sure how tricky it can be to figure out which all files are header files. It can be easier if all c and c++ header files follow standard file name convention.
Thoughts?
Using the Provices seems a good heuristic. But note that the Provides are based on the DT_SONAME. So you cannot simply match on file name. The library so name might be based on the filename, e.g. libfoo.so.3 but doesn't have to be. It also isn't based on the path of the file.
About there being possibly being too many library provides. Yes, that might happen, but is probably a packaging bug. The provides are generated for all ELF shared libraries. But if so, and libabigail does signal the ABI of some of those changed, while it shouldn't, then this is a good indication that the spec file should be updated to exclude those libraries by defining __provides_exclude.
Mark Wielaard pagure@pagure.io a =C3=A9crit:
Good catch. I'll have to handle filtering based on dt_soname then.
Sinny Kumari pagure@pagure.io a =C3=A9crit:
sinnykumari added a new comment to an issue you are following: `` Another case where we can avoid doing ABI comparison on a DSO, when - No header file is shipped in any of sub-packages (of course after excluding debuginfo and src rpms). No public header can mean that package is not meant for consumption by other packages. I am not sure how tricky it can be to figure out which all files are header files. It can be easier if all c and c++ header files follow standard file name convention. Thoughts?
sinnykumari added a new comment to an issue you are following: `` Another case where we can avoid doing ABI comparison on a DSO, when - No header file is shipped in any of sub-packages (of course after excluding debuginfo and src rpms). No public header can mean that package is not meant for consumption by other packages. I am not sure how tricky it can be to figure out which all files are header files. It can be easier if all c and c++ header files follow standard file name convention.
Yes, this makes sense to me too.
So basically, here is the plan:
whenever we detect a ABI change (related to a type), we'll look at the file where the type is defined. That file should be a header file. If that header file is part of the header files that are associated to the package (-devel or -headers package) then the type is meant to be "publicly consumed". So we just emit a report about the change.
Otherwise, it's a private type, so we should just suppress the change and not display it in the report.
We can implement this in addition to the filtering based on the RPM Requires.
I'll sum all this up in a subsequent comment.
Here is the proposal I am making to help better auto-detect (and ignore) private DSOs from RPMs.
--------------------------->8<--------------------------------------------------- * Auto-detecting private DSOs in abipkgdiff and task-abicheck
When the DT_SONAME of a given DSO is empty or is not included in the Requires of an RPM, then we can consider that the DSO is private to the package.
We can thus make abipkgdiff ignore DSOs which have DT_SONAME not present in the 'Requires' of the RPM. We need an option to deactivate this behaviour, though, probably something like --dont-ignore-private-dtsoname.
In addition to this first heuristic, a DSO with a DT_SONAME that is present in the Requires property of the RPM might have ABI changes impacting types that are meant to be 'private'. That is, types that are not meant to be consumed by any external binary. In that case, we need to detect that the type is defined in a (header) file that is not part of the set of header files associated to the package, i.e, the -devel and -headers associated packages.
Note that this second heuristic is already implemented by abipkgdiff and task-abicheck as the latter uses the --devel <devel-package> option of the former. But then, if no devel/headers package was provided then no ABI change is suppressed.
We'd need to change this current behaviour by saying that if no --devel option is provided then it means that the package has no binary that exports an ABI and so no ABI verification should take place. --------------------------->8<---------------------------------------------------
Here is the proposal I am making to help better auto-detect (and ignore) private DSOs from RPMs. [...] Thoughts?
I think you mean Provides where you say Requires in the proposal.
Also, when documenting this you might want to point to https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering to explain to packagers how to prevent auto provides that trigger abichecks.
Here is the proposal I am making to help better auto-detect (and ignore) private DSOs from RPMs. [...]
Agree with what Mark said. Other than that proposal sounds good to me.
@dodji, I see that libabigail-1.2 is now in updates-testing. Does it contain any fixes related to this ticket?
@dodji, I see that libabigail-1.2 is now in updates-testing.
Indeed. And it does contain some important general bug fixes.
Does it contain any fixes related to this ticket?
Sorry, not yet, Kamil.
I'll be working on this issue during this cycle, so presumably for the 1.3 version.
Sorry for the inconvenience.
So, I added support for filtering out private shared libraries based on the result of the "provides" property of the RPM, in the abipkgdiff tool of Libabigail, in the master branch of the git repository.
That is, by default, abipkgdiff looks at the shared libraries which SONAMEs are listed in the "provides" property of the RPM. Then all shared libraries which SONAME is not listed in the "provides" property are removed from the list of shared libraries that ought to be ABI-compared.
It's still possible to get the old behaviour where all shared libraries are ABI-verified, regardless of their being listed in the "provides", by using the new --private-dso option.
For instance, this is the output of abipkdfiff on the gap-pkg-io package:
$ abipkgdiff --d1 gap-pkg-io-debuginfo-4.4.6-4.fc27.x86_64.rpm --d2 gap-pkg-io-debuginfo-4.5.1-1.fc27.x86_64.rpm gap-pkg-io-4.4.6-4.fc27.x86_64.rpm gap-pkg-io-4.5.1-1.fc27.x86_64.rpm $
And as I said above, we can get the old behaviour back by using the new -private-dso option:
$ abipkgdiff --private-dso --d1 gap-pkg-io-debuginfo-4.4.6-4.fc27.x86_64.rpm --d2 gap-pkg-io-debuginfo-4.5.1-1.fc27.x86_64.rpm gap-pkg-io-4.4.6-4.fc27.x86_64.rpm gap-pkg-io-4.5.1-1.fc27.x86_64.rpm ================ changes of 'io.so'=============== Functions changes summary: 1 Removed, 0 Changed, 1 Added functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
1 Removed function:
'function Bag FuncIO_MasterPointerNumber(Bag, Bag)' {FuncIO_MasterPointerNumber}
1 Added function:
'function Bag FuncIO_IgnorePid(Bag, Bag)' {FuncIO_IgnorePid}
================ end of changes of 'io.so'===============
$
That feature should be available in the coming 1.3 version of Libabigail.
@sinnykumari: So, for the filtering feature to be complete as per the proposal I made earlier, I think what remains to be done is to avoid running the ABI comparison if a package has NO associated -devel package. In that case, we consider that the package exports no public ABI.
I think that ought to be implemented in task-abicheck itself, not in abipkgdiff.
What do you think?
@sinnykumari: So, for the filtering feature to be complete as per the proposal I made earlier, I think what remains to be done is to avoid running the ABI comparison if a package has NO associated -devel package. In that case, we consider that the package exports no public ABI. I think that ought to be implemented in task-abicheck itself, not in abipkgdiff. What do you think?
That's correct! I will start working on avoiding ABI comparison when no associated -devel package is available.
That's correct!=20
Cool, I wanted to make sure I wasn't too confused ;-)
I will start working on avoiding ABI comparison when no associated -devel package is available.
Oh, thank you!
By the way, I think that change (avoiding ABI comparison when no -devel pac= kage is around) can be made independantly from having the filtering based on "provides". In other words, you can do that even before libabigail 1.3 is released. The two useful independantly from each other, I think.
Is that correct?
Sinny Kumari pagure@pagure.io a =C3=A9crit: That's correct!=20 Cool, I wanted to make sure I wasn't too confused ;-) I will start working on avoiding ABI comparison when no associated -devel package is available. Oh, thank you! By the way, I think that change (avoiding ABI comparison when no -devel pac= kage is around) can be made independantly from having the filtering based on "provides". In other words, you can do that even before libabigail 1.3
Oh, thank you! By the way, I think that change (avoiding ABI comparison when no -devel pac= kage is around) can be made independantly from having the filtering based on "provides". In other words, you can do that even before libabigail 1.3
right!
is released. The two useful independantly from each other, I think. Is that correct?
Yes, both are independent task
@sinnykumari: So, for the filtering feature to be complete as per the proposal I made earlier, I think what remains to be done is to avoid running the ABI comparison if a package has NO associated -devel package. In that case, we consider that the package exports no public ABI. I think that ought to be implemented in task-abicheck itself, not in abipkgdiff. What do you think? That's correct! I will start working on avoiding ABI comparison when no associated -devel package is available
That's correct! I will start working on avoiding ABI comparison when no associated -devel package is available
Sorry about picking up this task after long time. Here is the PR - https://pagure.io/task-abicheck/pull-request/15
Login to comment on this ticket.