[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: knotifications_5.45.0-2_source.changes ACCEPTED into unstable



¡Hola Pino!

El 2018-05-13 a las 10:37 +0200, Pino Toscano escribió:
Still, this is misusing a tool for not what is designed for.  The ABI
compliance checker is, well, a tool for checking ABI, not for "checking
headers", and this means that the tool can change its way of working
any day, breaking the way it is (mis)used in this case.

While I understand your concern that we are misusing the tool, checking the headers is currently one of the provided options of the abi-compliance-checker functionality. Removing this functionality would not only break us, but any other developers using this option.

Also, neither dh-acc, acc, or any of the acc autopkgtest in sources
depend on a g++ compiler, so I don't see how this even works (unless
something else pulls it as side dependency, or a chroot with
build-essential is by chance reused for the test).

abi-compliance-checker depends on build-essential.

Furthermore, just parsing the headers is not a real-world test: acc
seems to discover the location of the headers used on its own, while
when using cmake/qmake the actual headers path will be added using
targets and/or variables, which is what needs to be tested (see
#877351 for example).

acc finds the location of the headers to check by reading the .acc file. Additionally, it sets the include path by using find on /usr/include and then looking for the filenames that are included from the header files.

It's true that this doesn't check for bugs in the cmake files, but as mentioned, it allows us to catch other issues (missing dependencies, includes of private headers, etc).

The bug you mention seems to be a good example of something that an additional test could catch.

Another point is that acc seems to use a C compiler when parsing the
headers, which means that it basically skips all the content of KF5
headers, demoting the acc run to a mere "do all the #include's work",
and nothing else.

acc does indeed call the gcc binary, but gcc automatically detects that it's a C++ file and uses the c++ backend.

The only exception to this is the empty file check, which is used for listing the system includes. If we wanted this check to also use g++, we would need to set the compiler, which we can't do using dh_acc, but we could do by doing an explicit acc call like this:
abi-compliance-checker -q -l $package -v1 $version -dump $acc_file \
   -dump-path $package_$version.abi -gcc-path /usr/bin/g++

Again: acc is the wrong tool for this job.

I'm all in favour in using a better tool for the job, if it existed. I could probably even work on such a tool.

For now, I consider that the value that the tests provide is larger than the problems of misusing the current tool.

I have added a comment in the tests' control files explaining the current situation.

Then let's have a _proper_ build test: add a sample source code (or
uses the examples, if the sources ship any), and build it with
cmake/qmake. This will be way more close to how libraries are actually
used, rather than just parsing their headers.

I'm not opposed on having this kind of tests, but they would need to be each manually written and maintained (ideally, upstream). Whereas the tests as done by acc allow us to do a general verification of the headers without having to manually write specific files for each package.

Happy hacking,
--
"If a pickpocket meets a saint, he sees only his pockets."
-- Kegley's Law
Saludos /\/\ /\ >< `/

Attachment: signature.asc
Description: PGP signature


Reply to: