Re: autodep8 test for C/C++ header
On martes, 8 de agosto de 2023 04:50:04 -03 Helmut Grohne wrote: > Hi Sune, > > On Tue, Aug 08, 2023 at 06:46:38AM -, Sune Vuorela wrote: > > I don't think this is a important problem that some headers might have > > special conditions for use. I'd rather have our developers spend time > > fixing other issues than satisfying this script. > > A while ago, I've performed a similar analysis for Python and given my > experience there, I disagree with you here. As far as I understand both > you and Peter, you argue that such an autodep integration would fail for > some package for various (valid) reasons. What Benjamin seems to propose > is adding support for an automated check that is opt-in (not opt-out). > As a developer, you have to explicitly enable it in order to use it. > Given the numbers presented by Benjamin and the examples presented by > both Peter and you, I expect that Benjamin's script would just work for > at least half of the packages. And for those where it just works, I see > it as a useful safety measure. > > > Is it a problem that Qt -dev packages also ships windows, mac or android > > specific addons headers? I don't think so. Rather the opposite. When > > doing cross platform work, it is nice that grepping across the includes, > > I also see some of the platformspecific functions in separate files. > > > > Is it a problem that a header file is also shipped that provides > > integration with other-big-thing but 99% of developres/downstream users > > don't care about and no Depends is in place? I don't think that's a > > problem. I'd rather have the header available for the 1% than having to > > create an extra -dev package just for that. > > > > Debian development resources is a finite resource, so let's not waste > > it. > > This goes both ways. The team at Canonical is currently dealing with > lots of failures that are tangential to time64. Let's not waste their > time either. I'm experiencing a similar issue with my work on > /usr-merge. In order to complete that transition in a safe way, I need > file conflicts to be precisely declared, but people frequently introduce > new ones and some even argue about their severity. That's also "wasting" > my time. > > So from my point of view, we need to strike a balance here. Benjamin is > offering an opt-in tool to reduce his waste time. Having half of the > packages opt into it, would already reduce QA work significantly, so > that sounds like a very good measure to me. > > Can we agree on moving forward with this while not forcing it onto each > and every package? I can definitely agree with this as long as it's an opt-in. In fact it could be useful to run it from time to time or even have a way to say "run but ignore this or that case". Of course this might be trickier: it is totally possible for a header to declare conditional includes depending on the platform. signature.asc Description: This is a digitally signed message part.
Re: autodep8 test for C/C++ header
On Wed, 2023-08-09 at 18:30 +0300, Adrian Bunk wrote: > On Wed, Aug 09, 2023 at 02:26:17PM +0800, Paul Wise wrote: > > On Tue, 2023-08-08 at 18:32 +0300, Adrian Bunk wrote: > > > > > Manual opt-in for our > 11k -dev packages is a significant cost > > > that would have to be justified by the people who oppose opt-out. > > > > You could use the Janitor to do automatic opt-in where it works, > > IIRC the Janitor runs autopkgtests before filing merge requests, > > so it could easily try the tests and enable them when they work. > > > > Or the other way, enable them and have the Janitor submit merge > > requests to turn them off where they don't work. > > The cases where they would fail are either false positives or RC bugs. > > Janitor merge requests to silence the failures for all actual RC bugs > would not make sense. > > The immediate benefit of such a test would be a review of all failing > cases and filing of RC bugs (which might be > 200) for all that look > like bugs. > > After the MBF it should be clear how many RC bugs actually exist > in practice. The information whether a failure is a RC bug or a false positive can be extracted from check-armhf-time_t [1] - at least for the packages that were analyzed. There are still 1385 packages [2] that are not analyzed yet. [1] https://salsa.debian.org/vorlon/armhf-time_t/-/blob/main/check-armhf-time_t [2] curl https://wiki.debian.org/ArmhfTimeTTodo?action=raw | grep -v merge_requests | wc -- Benjamin Drung Debian & Ubuntu Developer
Re: autodep8 test for C/C++ header
On Wed, Aug 09, 2023 at 02:26:17PM +0800, Paul Wise wrote: > On Tue, 2023-08-08 at 18:32 +0300, Adrian Bunk wrote: > > > Manual opt-in for our > 11k -dev packages is a significant cost > > that would have to be justified by the people who oppose opt-out. > > You could use the Janitor to do automatic opt-in where it works, > IIRC the Janitor runs autopkgtests before filing merge requests, > so it could easily try the tests and enable them when they work. > > Or the other way, enable them and have the Janitor submit merge > requests to turn them off where they don't work. The cases where they would fail are either false positives or RC bugs. Janitor merge requests to silence the failures for all actual RC bugs would not make sense. The immediate benefit of such a test would be a review of all failing cases and filing of RC bugs (which might be > 200) for all that look like bugs. After the MBF it should be clear how many RC bugs actually exist in practice. > bye, > pabs cu Adrian
Re: autodep8 test for C/C++ header
On Tue, 2023-08-08 at 18:32 +0300, Adrian Bunk wrote: > On Tue, Aug 08, 2023 at 09:19:16AM -0300, Antonio Terceiro wrote: > > On Tue, Aug 08, 2023 at 11:35:01AM +0300, Adrian Bunk wrote: > > > On Tue, Aug 08, 2023 at 06:46:38AM -, Sune Vuorela wrote: > > > > On 2023-08-07, Benjamin Drung wrote: > > > > > while working a whole week on fixing failing C/C++ header compilations > > > > > for armhf time_t [1], I noticed a common pattern: The library -dev > > > > > packages was missing one or more dependencies on another -dev package. > > > > > Over 200 -dev packages are affected. > > > > > > > > I don't think this is a important problem that some headers might have > > > > special conditions for use. I'd rather have our developers spend time > > > > fixing other issues than satisfying this script. > > > > ... > > > > > > There are many actual bugs it would catch, that are currently only > > > caught later manually (sometimes through bug reports from users in > > > stable). > > > > > > There are special cases that might result in false positives. > > > > > > Numbers for bugs found and false positives should help determine whether > > > it should be opt-in or opt-out. > > > > While providing this for packages to use is a great idea, this will have > > to be opt-in. Imposing this on maintainers has a significant technical > > and social cost, specially in the case of packages where the defaults > > don't work correctly, that I am not willing to pay. > > ... > > Manual opt-in for our > 11k -dev packages is a significant cost > that would have to be justified by the people who oppose opt-out. > > Are the > 200 affected -dev packages > > 200 RC bugs and a dozen false positives, > or are they > 200 false positives and a dozen RC bugs? There are > 200 -dev packages that have missing dependencies. Some of them will compile with the added missing dependencies, but some of them need more quirks. So they are a mixed bag. My gut feeling from the armhf work last week is that 70% just need the additional dependencies. -- Benjamin Drung Debian & Ubuntu Developer
Re: autodep8 test for C/C++ header
On Tue, 2023-08-08 at 11:42 +0300, Adrian Bunk wrote: > An additional check with some overlap would be whether > pkgconf --cflags .pc > returns 0 for every pkgconfig file in a package. piuparts runs adequate, which runs something similar: pkg-config --exists --print-errors .pc It emits missing-pkgconfig-dependency when appropriate: https://piuparts.debian.org/sid/missing_pkgconfig-dependency_issue.html There are 42 packages in sid failing this test. -- bye, pabs https://wiki.debian.org/PaulWise signature.asc Description: This is a digitally signed message part
Re: autodep8 test for C/C++ header
On Tue, 2023-08-08 at 18:32 +0300, Adrian Bunk wrote: > Manual opt-in for our > 11k -dev packages is a significant cost > that would have to be justified by the people who oppose opt-out. You could use the Janitor to do automatic opt-in where it works, IIRC the Janitor runs autopkgtests before filing merge requests, so it could easily try the tests and enable them when they work. Or the other way, enable them and have the Janitor submit merge requests to turn them off where they don't work. -- bye, pabs https://wiki.debian.org/PaulWise signature.asc Description: This is a digitally signed message part
Re: autodep8 test for C/C++ header
Hi Sune, On Tue, Aug 08, 2023 at 06:46:38AM -, Sune Vuorela wrote: > I don't think this is a important problem that some headers might have > special conditions for use. I'd rather have our developers spend time > fixing other issues than satisfying this script. A while ago, I've performed a similar analysis for Python and given my experience there, I disagree with you here. As far as I understand both you and Peter, you argue that such an autodep integration would fail for some package for various (valid) reasons. What Benjamin seems to propose is adding support for an automated check that is opt-in (not opt-out). As a developer, you have to explicitly enable it in order to use it. Given the numbers presented by Benjamin and the examples presented by both Peter and you, I expect that Benjamin's script would just work for at least half of the packages. And for those where it just works, I see it as a useful safety measure. > Is it a problem that Qt -dev packages also ships windows, mac or android > specific addons headers? I don't think so. Rather the opposite. When > doing cross platform work, it is nice that grepping across the includes, > I also see some of the platformspecific functions in separate files. > > Is it a problem that a header file is also shipped that provides > integration with other-big-thing but 99% of developres/downstream users > don't care about and no Depends is in place? I don't think that's a > problem. I'd rather have the header available for the 1% than having to > create an extra -dev package just for that. > > Debian development resources is a finite resource, so let's not waste > it. This goes both ways. The team at Canonical is currently dealing with lots of failures that are tangential to time64. Let's not waste their time either. I'm experiencing a similar issue with my work on /usr-merge. In order to complete that transition in a safe way, I need file conflicts to be precisely declared, but people frequently introduce new ones and some even argue about their severity. That's also "wasting" my time. So from my point of view, we need to strike a balance here. Benjamin is offering an opt-in tool to reduce his waste time. Having half of the packages opt into it, would already reduce QA work significantly, so that sounds like a very good measure to me. Can we agree on moving forward with this while not forcing it onto each and every package? Helmut
Re: autodep8 test for C/C++ header
On Tue, Aug 08, 2023 at 09:19:16AM -0300, Antonio Terceiro wrote: > On Tue, Aug 08, 2023 at 11:35:01AM +0300, Adrian Bunk wrote: > > On Tue, Aug 08, 2023 at 06:46:38AM -, Sune Vuorela wrote: > > > On 2023-08-07, Benjamin Drung wrote: > > > > while working a whole week on fixing failing C/C++ header compilations > > > > for armhf time_t [1], I noticed a common pattern: The library -dev > > > > packages was missing one or more dependencies on another -dev package. > > > > Over 200 -dev packages are affected. > > > > > > I don't think this is a important problem that some headers might have > > > special conditions for use. I'd rather have our developers spend time > > > fixing other issues than satisfying this script. > > >... > > > > There are many actual bugs it would catch, that are currently only > > caught later manually (sometimes through bug reports from users in > > stable). > > > > There are special cases that might result in false positives. > > > > Numbers for bugs found and false positives should help determine whether > > it should be opt-in or opt-out. > > While providing this for packages to use is a great idea, this will have > to be opt-in. Imposing this on maintainers has a significant technical > and social cost, specially in the case of packages where the defaults > don't work correctly, that I am not willing to pay. >... Manual opt-in for our > 11k -dev packages is a significant cost that would have to be justified by the people who oppose opt-out. Are the > 200 affected -dev packages > 200 RC bugs and a dozen false positives, or are they > 200 false positives and a dozen RC bugs? cu Adrian
Re: autodep8 test for C/C++ header
On Tue, Aug 08, 2023 at 11:35:01AM +0300, Adrian Bunk wrote: > On Tue, Aug 08, 2023 at 06:46:38AM -, Sune Vuorela wrote: > > On 2023-08-07, Benjamin Drung wrote: > > > while working a whole week on fixing failing C/C++ header compilations > > > for armhf time_t [1], I noticed a common pattern: The library -dev > > > packages was missing one or more dependencies on another -dev package. > > > Over 200 -dev packages are affected. > > > > I don't think this is a important problem that some headers might have > > special conditions for use. I'd rather have our developers spend time > > fixing other issues than satisfying this script. > >... > > There are many actual bugs it would catch, that are currently only > caught later manually (sometimes through bug reports from users in > stable). > > There are special cases that might result in false positives. > > Numbers for bugs found and false positives should help determine whether > it should be opt-in or opt-out. While providing this for packages to use is a great idea, this will have to be opt-in. Imposing this on maintainers has a significant technical and social cost, specially in the case of packages where the defaults don't work correctly, that I am not willing to pay. With regards to supporting different corner cases, autodep8 has a standard interface for packages to provide configuration to the runners, and that is documented under "PACKAGE‐SPECIFIC CONFIGURATION" in the manpage. signature.asc Description: PGP signature
Re: autodep8 test for C/C++ header
On Mon, Aug 07, 2023 at 06:43:36PM +, Benjamin Drung wrote: >... > I propose to add an autodep8 test for C/C++ header files that tries to > compile the header file. This will catch issues like missing > dependencies and other issues. >... An additional check with some overlap would be whether pkgconf --cflags .pc returns 0 for every pkgconfig file in a package. That would also catch a common kind of bugs. cu Adrian
Re: autodep8 test for C/C++ header
On Tue, Aug 08, 2023 at 06:46:38AM -, Sune Vuorela wrote: > On 2023-08-07, Benjamin Drung wrote: > > while working a whole week on fixing failing C/C++ header compilations > > for armhf time_t [1], I noticed a common pattern: The library -dev > > packages was missing one or more dependencies on another -dev package. > > Over 200 -dev packages are affected. > > I don't think this is a important problem that some headers might have > special conditions for use. I'd rather have our developers spend time > fixing other issues than satisfying this script. >... There are many actual bugs it would catch, that are currently only caught later manually (sometimes through bug reports from users in stable). There are special cases that might result in false positives. Numbers for bugs found and false positives should help determine whether it should be opt-in or opt-out. > /Sune cu Adrian
Re: autodep8 test for C/C++ header
On Mon, Aug 07, 2023 at 09:17:18PM +, Benjamin Drung wrote: > On Mon, 2023-08-07 at 22:52 +0300, Peter Pentchev wrote: >... > > 1) The library has a "main" header file that must be included before > >any of the others, and it does not come first in lexicographical > >order. It may define typedefs and structure definitions that > >the other header files can use, it may define preprocessor symbols > >that reflect the availability of this or that system header file or > >type; there are also other ways in which another header file > >distributed by the -dev package may depend on the main one. > > In this case the non-"main" header could just include the "main" header > as first step. Alternatively, an option to specify headers that should > be included first could be added to the check script. >... The opposite problem also exists, where it is documented that only one/few "main" headers are supposed to be included directly by users and the other headers are internal headers only to be included by the "main" headers (or by other internal headers). glibc and GNOME would be examples for this. cu Adrian
Re: autodep8 test for C/C++ header
On 2023-08-07, Benjamin Drung wrote: > while working a whole week on fixing failing C/C++ header compilations > for armhf time_t [1], I noticed a common pattern: The library -dev > packages was missing one or more dependencies on another -dev package. > Over 200 -dev packages are affected. I don't think this is a important problem that some headers might have special conditions for use. I'd rather have our developers spend time fixing other issues than satisfying this script. Is it a problem that Qt -dev packages also ships windows, mac or android specific addons headers? I don't think so. Rather the opposite. When doing cross platform work, it is nice that grepping across the includes, I also see some of the platformspecific functions in separate files. Is it a problem that a header file is also shipped that provides integration with other-big-thing but 99% of developres/downstream users don't care about and no Depends is in place? I don't think that's a problem. I'd rather have the header available for the 1% than having to create an extra -dev package just for that. Debian development resources is a finite resource, so let's not waste it. /Sune
Re: autodep8 test for C/C++ header
Hi, Quoting Benjamin Drung (2023-08-07 20:43:36) > while working a whole week on fixing failing C/C++ header compilations for > armhf time_t [1], I noticed a common pattern: The library -dev packages was > missing one or more dependencies on another -dev package. Over 200 -dev > packages are affected. > > I propose to add an autodep8 test for C/C++ header files that tries to > compile the header file. This will catch issues like missing > dependencies and other issues. > > Here is a draft for the autopkgtest check script that takes a binary > -dev package as parameter: > https://github.com/bdrung/bdrung-scripts/blob/compile-header-check/compile-header-check > > What do you think? Should I proceed developing and packaging this check > script and submit support for it in autodep8? yes please! I've manually written similar scripts for my own packages in the past with the same reason: find missing dependencies of -dev packages. Here is an example of what I wrote for ros-* packages: https://sources.debian.org/src/ros-image-pipeline/1.17.0-1/debian/tests/compilation/ Thanks! cheers, josch signature.asc Description: signature
Re: autodep8 test for C/C++ header
On Mon, 2023-08-07 at 22:52 +0300, Peter Pentchev wrote: > On Mon, Aug 07, 2023 at 06:43:36PM +, Benjamin Drung wrote: > > Hi, > > > > while working a whole week on fixing failing C/C++ header compilations > > for armhf time_t [1], I noticed a common pattern: The library -dev > > packages was missing one or more dependencies on another -dev package. > > Over 200 -dev packages are affected. > > > > I propose to add an autodep8 test for C/C++ header files that tries to > > compile the header file. This will catch issues like missing > > dependencies and other issues. > > > > Here is a draft for the autopkgtest check script that takes a binary > > -dev package as parameter: > > https://github.com/bdrung/bdrung-scripts/blob/compile-header-check/compile-header-check > > > > What do you think? Should I proceed developing and packaging this check > > script and submit support for it in autodep8? > > > > [1] https://salsa.debian.org/vorlon/armhf-time_t/-/merge_requests/103 > > Whlie it does seem an interesting tool at first glance (and thanks for > doing the work and presenting a proof of concept), I can think of > several cases when compilation of the concatenated header files would > fail: Yes, I expect that several packages would fail to compile and the check script would need several knobs to tweak (skip certain headers, add or exclude certain include directories, define a specific C/C++ standand version, use only C and not C++, etc). A set of common approaches to solve such failures should be added to the documentation of the check script (probably including skipping the whole check). > 1) The library has a "main" header file that must be included before >any of the others, and it does not come first in lexicographical >order. It may define typedefs and structure definitions that >the other header files can use, it may define preprocessor symbols >that reflect the availability of this or that system header file or >type; there are also other ways in which another header file >distributed by the -dev package may depend on the main one. In this case the non-"main" header could just include the "main" header as first step. Alternatively, an option to specify headers that should be included first could be added to the check script. > 2) As a variation of the above, the -dev package may distribute >the full set of header files included in the library, and some of >them may only be included if certain preprocessor symbols are >defined. Since their use is guarded by conditionals, they are >allowed to unconditionally include system-specific header files >that are only available on e.g. Windows or NetBSD or Darwin, etc. >Unconditionally compiling the contents of those files would fail. The -dev packages could drop shipping unneeded headers (e.g. Windows or BSD specific ones), some headers could be excluded by a parameter to the check script, and/or the check script could allow defining some names (pass -D name=definition to gcc). > 3) The -dev package may distribute the full set of header files >included in the library, and some of them may be mutually exclusive >and impossible to combine. For example, a header file may include >either this or that other header file based on preprocessor >defintions (OS type, features enabled, etc), and the included >files may both define a function with the same name, but different >contents. That one is trickier to reflect in the check script. > 4) Some of the library's header files may not be supposed to be >included in all cases; the library's -dev package may suggest >(but not depend on or recommend) another -dev package as >an optional dependency, and a library's header file may >unconditionally include some header file from the latter package. Either exclude the header from the check script or not ship this header file in the -dev package. > All of these cases are things I've seen in complex libraries with > many header files; maybe not all of them can be found in Debian > right now. When you look at check-armhf-time_t [2] you will find all those mentioned cases and some more. > So... yeah. Thanks for your work, I know you mean well and you are > trying to make the life of Debian developers better, but this > particular approach will likely fail on a non-trivial set of > Debian -dev packages. I expect this approach to work for most packages, fail for several packages due to bugs in the package (missing dependencies and a bunch of other issues like missing double include protection) and some will fail with the reasons above. According to [3] there are 5360 C/C++ -dev package and only 1900 -dev packages (35%) did not compile out of the box [4]. The majority of those 1900 -dev packages do not compile due to bugs in the package (judging from the workaround that we apply in [2]). IMO having this check for 90% of the packages to catch bugs and disabling it for 10% is
Re: autodep8 test for C/C++ header
On Mon, Aug 07, 2023 at 06:43:36PM +, Benjamin Drung wrote: > Hi, > > while working a whole week on fixing failing C/C++ header compilations > for armhf time_t [1], I noticed a common pattern: The library -dev > packages was missing one or more dependencies on another -dev package. > Over 200 -dev packages are affected. > > I propose to add an autodep8 test for C/C++ header files that tries to > compile the header file. This will catch issues like missing > dependencies and other issues. > > Here is a draft for the autopkgtest check script that takes a binary > -dev package as parameter: > https://github.com/bdrung/bdrung-scripts/blob/compile-header-check/compile-header-check > > What do you think? Should I proceed developing and packaging this check > script and submit support for it in autodep8? > > [1] https://salsa.debian.org/vorlon/armhf-time_t/-/merge_requests/103 Whlie it does seem an interesting tool at first glance (and thanks for doing the work and presenting a proof of concept), I can think of several cases when compilation of the concatenated header files would fail: 1) The library has a "main" header file that must be included before any of the others, and it does not come first in lexicographical order. It may define typedefs and structure definitions that the other header files can use, it may define preprocessor symbols that reflect the availability of this or that system header file or type; there are also other ways in which another header file distributed by the -dev package may depend on the main one. 2) As a variation of the above, the -dev package may distribute the full set of header files included in the library, and some of them may only be included if certain preprocessor symbols are defined. Since their use is guarded by conditionals, they are allowed to unconditionally include system-specific header files that are only available on e.g. Windows or NetBSD or Darwin, etc. Unconditionally compiling the contents of those files would fail. 3) The -dev package may distribute the full set of header files included in the library, and some of them may be mutually exclusive and impossible to combine. For example, a header file may include either this or that other header file based on preprocessor defintions (OS type, features enabled, etc), and the included files may both define a function with the same name, but different contents. 4) Some of the library's header files may not be supposed to be included in all cases; the library's -dev package may suggest (but not depend on or recommend) another -dev package as an optional dependency, and a library's header file may unconditionally include some header file from the latter package. All of these cases are things I've seen in complex libraries with many header files; maybe not all of them can be found in Debian right now. So... yeah. Thanks for your work, I know you mean well and you are trying to make the life of Debian developers better, but this particular approach will likely fail on a non-trivial set of Debian -dev packages. G'luck, Peter -- Peter Pentchev r...@ringlet.net r...@debian.org p...@storpool.com PGP key:http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13 signature.asc Description: PGP signature
