lvc / abi-compliance-checker

A tool for checking backward API/ABI compatibility of a C/C++ library
https://lvc.github.io/abi-compliance-checker/
GNU Lesser General Public License v2.1
625 stars 78 forks source link

Remove strange "familiarDirs" logic leading to "random" wrong false p… #75

Closed Romain-Geissler-1A closed 6 years ago

Romain-Geissler-1A commented 6 years ago

…ositive depending on your folder names.

Hi,

You will most likely want to discuss this. I am having troubles with a migration from compliance checker version 1.99.x to any version >= 2.0.

Let's consider this scenario:

Now I am working on libB and want to deliver a version 2. Actually nothing has changed between version 1 and version 2, only the version and nothing else. Everything is fully compatible both at binary and source level. Let's say I want to use compliance checker to check that BEFORE putting version 2 inside /delivery/libB/v1: let's say I work in /my/user/folder. Calling compliance checker to compare /delivery/libB/v1 (version 1) and /my/user/folder (version 2) by using /delivery/libA/v1 as a dependency in both cases, compliance checker will decide to include the headers of A version 1 in the first case, and will not in the second case. And here you end up with a false positive. If instead you decide to first install the library in /delivery/libB/v2 and compare v1 and v2 again, then it will work. Conclusion: the path where you run compliance checker matters a lot and can create false positives.

The reason is: this familiarDirs function which from what I understood will decides to include a particular header if a common prefix with the path of the header it was included from is found. I don't understand why this common prefix logic is there. Are you trying to exclude from the analysis what you consider are system headers, or headers not from the project we try to analyse ? If yes, this is a behavior that is not always wanted, and I think it should be made configurable. Knowing if a library should or should not be consider part of the project to analyse is far more complex than trying to find common prefix, or checking if we used #include <...> vs #include "...". At least the analysis from compliance checker should not be made that dependent on paths having common prefixes or not, here I got a lot of troubles understanding where the issue came from between strictly 2 identical libraries, just not delivered in the same folder.

Cheers, Romain

lvc commented 6 years ago

This is needed for --dump-system option when you are dumping whole system (single version). Let's rewrite it like this:

if(($In::Opt{"DumpSystem"} and familiarDirs($RegDir, $Dir)) 
or $RecursiveIncludes{$LVer}{$RegHeader}{$RecInc}!=1)
{ # in the same directory or included by #include "..."
    $In::Desc{$LVer}{"TargetHeader"}{getFilename($RecInc)} = 1;
}

Please fix the pull request.

Thank you.

Romain-Geissler-1A commented 6 years ago

Thanks for the review. I have just updated the pull request with your advise.

lvc commented 6 years ago

Thank you!

Romain-Geissler-1A commented 6 years ago

Thanks

On our side, we still have applied the old patch that removes familiarDirs entirely since it has been working for months. Any plan for a release so that we can use an unpatched official release ?