Re: DEB_BUILD_OPTIONS=nowerror

[Helmut Grohne]

Hi Steve,

On Mon, Feb 27, 2023 at 04:39:29PM -0800, Steve Langasek wrote:
> Well, I'm not seeking to stand in the way of the work, so much as trying to
> make sure this is the most useful work to be doing to meet the actual use
> cases.

Thank you. I see that you are seeking a better solution and looking at
the problem from an angle that I didn't have.

In general, I don't see much consensus on my proposed option nor other
people explaining that it'd be useful to them.

> I can see that for bootstrapping a new architecture, it will sometimes be
> useful to use a toolchain newer than the one that is currently default in
> Debian, and as a result it is useful to also be able to bypass new stricter
> -Werror behavior from gcc upstream that breaks compatibility with existing
> code.

Good.

> It isn't clear to me from the discussion history that this is the actual use
> case at issue.  But supposing it is, that's one use case; and it's a use
> case that can be addressed without having to make any per-package changes
> and without having to make any changes to dpkg-buildflags interfaces by
> exporting
> 
>   DEB_CFLAGS_APPEND=-Wno-error
>   DEB_CXXFLAGS_APPEND=-Wno-error
> 
> as part of the bootstrap build environment, for all packages.

Thank you for proposing this solution that I did not see. It certainly
has beauty in that it really solves the problem at hand in a simple way
without expanding existing interfaces. I'm not sure whether it works in
practice, but I think that patches for making it work would probably be
accepted in the relevant packages if it doesn't.

To me, this looks like a better solution of my original problem.

Your persistence on this matter is welcome.

Helmut

Re: DEB_BUILD_OPTIONS=nowerror

[Colin Watson]

On Tue, Feb 28, 2023 at 11:10:24PM +0100, Philipp Kern wrote:
> On 28.02.23 20:34, Steve Langasek wrote:
> > But it's not practical to do CI -Werror builds; when we do
> > out-of-archive rebuilds for all architectures, it's a significant
> > committment of resources and each rebuild takes about a month to
> > complete (on the slowest archs).  And to be able to effectively
> > analyze build results to identify Werror-related failures with high
> > signal would require two parallel builds, one with and without the
> > flag, built against the same baseline.
> 
> That you are so resource constrained here surprises me a little. I can see
> that for Debian, but I'm surprised that Ubuntu is affected as well.
> Especially as you'd think that this could also be done within virtualization
> - the evaluation here is mostly around running the compiler and checking its
> errors, not so much about running tests accurately on real hardware.

All Ubuntu builds are virtualized.  For most architectures that's
OpenStack VMs on real hardware of the appropriate architecture; riscv64
builds currently run in emulated VMs on x86 hardware.

I have graphs handy, so I can say that most Ubuntu architectures
complete a full rebuild much more quickly than Steve indicated.  The
last time we did this, we started four parallel full rebuilds on most
architectures, and two on riscv64.  After these were started, the build
queues cleared on amd64 in about three days, ppc64el in five,
arm64+armhf (which share builders) in about seven, and s390x in about
nine.  That's including other normal activity on the same builders at
the same time.  Some of these (especially s390x) would be much faster
except that there are some unreliabilities in the inter-build VM reset
mechanism which caused failures and meant we weren't using anything like
our full build farm capacity; usually not so much of a problem in
practice, but full rebuilds tend to involve dispatching lots of small
builds and stressing that mechanism more than usual, and also we were
running this over the end-of-year holidays when not many people were
around to babysit things.

There are definitely various ways we can improve this further, which
aren't especially on-topic for debian-devel, but nevertheless this means
that on everything except riscv64 Ubuntu can do a single full rebuild in
a couple of days (with a bit of fuzz for the small number of multi-day
builds in the archive - this is just considering how long the build
queues take to drain).

The very long pole in the tent, though, is those emulated builds on
riscv64, which did indeed take rather more than a month to clear its
build queues last time, even though it was only running two full
rebuilds rather than four.  I don't think we're going to be able to get
real hardware with the hypervisor extension particularly soon, but we
may be able to throw some more x86 hardware at it soonish to mitigate
the problem.

-- 
Colin Watson (he/him)  [cjwat...@debian.org]

Re: DEB_BUILD_OPTIONS=nowerror

[Philipp Kern]

On 28.02.23 20:34, Steve Langasek wrote:

This is conceptually interesting to me.  In practice, I don't see us using
this in Ubuntu.  We have per-architecture differences from Debian (ppc64el
building with -O3 by default; riscv64 being a release architecture where it
isn't in Debian) that make it interesting to pick up on per-architecture
build failures caught by -Werror and not without.  But it's not practical to
do CI -Werror builds; when we do out-of-archive rebuilds for all
architectures, it's a significant committment of resources and each rebuild
takes about a month to complete (on the slowest archs).  And to be able to
effectively analyze build results to identify Werror-related failures with
high signal would require two parallel builds, one with and without the
flag, built against the same baseline.


That you are so resource constrained here surprises me a little. I can 
see that for Debian, but I'm surprised that Ubuntu is affected as well. 
Especially as you'd think that this could also be done within 
virtualization - the evaluation here is mostly around running the 
compiler and checking its errors, not so much about running tests 
accurately on real hardware.


Kind regards
Philipp Kern

Re: DEB_BUILD_OPTIONS=nowerror

[Steve Langasek]

On Tue, Feb 28, 2023 at 06:05:13PM +0100, Sven Mueller wrote:

> > It isn't clear to me from the discussion history that this is the actual
> > use case at issue.  But supposing it is, that's one use case; and it's a
> > use case that can be addressed without having to make any per-package
> > changes and without having to make any changes to dpkg-buildflags
> > interfaces by exporting

> >   DEB_CFLAGS_APPEND=-Wno-error
> >   DEB_CXXFLAGS_APPEND=-Wno-error

> > as part of the bootstrap build environment, for all packages.

> Now, if all packages would just use the flags from dpkg-buildflags as is,
> or as the final part of CFLAGS/CXXFLAGS, that would be nice. However, IME,
> that is not always the case and some maintainers append -Werror in
> particular.

This situation is not improved by the proposed added flag, because in both
cases all that changes is the output of dpkg-buildflags.  Packages that are
not respecting the dpkg-buildflags interface will be problematic independent
of this proposal.

> Following this discussion, I fear we might not reach consensus. But my ideal
> (strong) suggestion to package maintainers would be:

> 1) Don't use -Werror (or equivalent for your packages language) during
> normal
> builds (i.e. on buildd)

> 2) Do use -Werror via some mechanism (DEB_CFLAGS_APPEND)? during CI builds

> 3) Actually utilize CI builds to detect any breakages early.

This is conceptually interesting to me.  In practice, I don't see us using
this in Ubuntu.  We have per-architecture differences from Debian (ppc64el
building with -O3 by default; riscv64 being a release architecture where it
isn't in Debian) that make it interesting to pick up on per-architecture
build failures caught by -Werror and not without.  But it's not practical to
do CI -Werror builds; when we do out-of-archive rebuilds for all
architectures, it's a significant committment of resources and each rebuild
takes about a month to complete (on the slowest archs).  And to be able to
effectively analyze build results to identify Werror-related failures with
high signal would require two parallel builds, one with and without the
flag, built against the same baseline.  So since this is infeasible, if
Debian decides to pass -Wno-error by default from dpkg-buildflags, we might
find ourselves diverging in Ubuntu.

-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developer   https://www.debian.org/
slanga...@ubuntu.com vor...@debian.org


signature.asc
Description: PGP signature

Re: DEB_BUILD_OPTIONS=nowerror

[Sven Mueller]

Am 2023-02-28 01:39, schrieb Steve Langasek:
[some precursor


I can see that for bootstrapping a new architecture, it will sometimes 
be
useful to use a toolchain newer than the one that is currently default 
in
Debian, and as a result it is useful to also be able to bypass new 
stricter
-Werror behavior from gcc upstream that breaks compatibility with 
existing

code.


I agree, this is one use case.

It isn't clear to me from the discussion history that this is the 
actual use
case at issue.  But supposing it is, that's one use case; and it's a 
use
case that can be addressed without having to make any per-package 
changes

and without having to make any changes to dpkg-buildflags interfaces by
exporting

  DEB_CFLAGS_APPEND=-Wno-error
  DEB_CXXFLAGS_APPEND=-Wno-error

as part of the bootstrap build environment, for all packages.


Now, if all packages would just use the flags from dpkg-buildflags as 
is,
or as the final part of CFLAGS/CXXFLAGS, that would be nice. However, 
IME,
that is not always the case and some maintainers append -Werror in 
particular.


Of course, dpkg-buildflags also exports flags for other languages than 
C and
C++ (and should do), so if you want to have full package coverage you 
would
want your set of _APPEND variables to match the set of per-language 
flags
that dpkg-buildflags already handles.  Having to export 7 variables 
instead
of 1 is annoying.  But it also doesn't require reaching consensus on a 
new
interface in dpkg.  And I remain unconvinced that the particular 
proposed

interface is the right way around for Debian at large.


Following this discussion, I fear we might not reach consensus. But my 
ideal

(strong) suggestion to package maintainers would be:

1) Don't use -Werror (or equivalent for your packages language) during 
normal

builds (i.e. on buildd)

2) Do use -Werror via some mechanism (DEB_CFLAGS_APPEND)? during CI 
builds


3) Actually utilize CI builds to detect any breakages early.


(1) helps in multiple cases, all of which are rebuilds of some sort.

Security: Minimize the patch to the package if the compiler was updated 
since

your package was last built.

Derived distros: I see this quite regularly at Google - packages not 
rebuilt
in Debian for months, now fail to build with newer gcc. While we use 
testing
as our base (so usually not *too* far off from unstable), we do rebuild 
packages
in testing more often than Debian would. Not strictly necessary in most 
cases,
but most of the time, if package bar build-depends on libfoo-dev and 
that gets

an update, bar will usually be rebuilt (not always, but whatever).

Porting (as mentioned by Steve and others) - Porting to other 
architectures

often requires a different (newer) compiler version, which might lead to
failures with -Werror.

Cheers,
Sven

Re: DEB_BUILD_OPTIONS=nowerror

[Steve Langasek]

On Mon, Feb 27, 2023 at 03:06:25PM -0700, Sam Hartman wrote:
> > "Steve" == Steve Langasek  writes:
> Steve> If this is for people doing out-of-archive builds who don't
> Steve> want to deal with failures from -Werror, I can see how having
> Steve> a single environment toggle is useful to them; but it doesn't
> Steve> seem useful to *Debian* when such out-of-archive rebuilds
> Steve> don't correspond to the official builds.  E.g. if they're
> Steve> test builds for new toolchains, knowing that the package
> Steve> *could* build, with options Debian doesn't actually use, is
> Steve> of limited use.  (If you build twice, once with once without
> Steve> and file bugs on packages where the results differ, I guess
> Steve> that's one use, but involves a lot of manual labor.)

> In the general case I agree.
> But we have the specific case of cross-bootstrapping.
> They want to be able to do builds to bootstrap and they want to have an
> option they can pass into the build to ask  debian/rules not to include
> -Werror.

> I suspect Helmut believes he can get patches accepted in the packages
> where this is most important to honor the option.
> Given his track record, I bet he's right.

> So, we have a team in Debian wanting a interface sufficiently
> standardized for them to do their work.
> I think we have confidence that once we agree on a interface they can
> produce appropriate consumers of the interface as well as
> implementations of the interface.

> I think we should have a high bar for standing in the way of this kind
> of work.

Well, I'm not seeking to stand in the way of the work, so much as trying to
make sure this is the most useful work to be doing to meet the actual use
cases.

I can see that for bootstrapping a new architecture, it will sometimes be
useful to use a toolchain newer than the one that is currently default in
Debian, and as a result it is useful to also be able to bypass new stricter
-Werror behavior from gcc upstream that breaks compatibility with existing
code.

It isn't clear to me from the discussion history that this is the actual use
case at issue.  But supposing it is, that's one use case; and it's a use
case that can be addressed without having to make any per-package changes
and without having to make any changes to dpkg-buildflags interfaces by
exporting

  DEB_CFLAGS_APPEND=-Wno-error
  DEB_CXXFLAGS_APPEND=-Wno-error

as part of the bootstrap build environment, for all packages.

Of course, dpkg-buildflags also exports flags for other languages than C and
C++ (and should do), so if you want to have full package coverage you would
want your set of _APPEND variables to match the set of per-language flags
that dpkg-buildflags already handles.  Having to export 7 variables instead
of 1 is annoying.  But it also doesn't require reaching consensus on a new
interface in dpkg.  And I remain unconvinced that the particular proposed
interface is the right way around for Debian at large.

-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developer   https://www.debian.org/
slanga...@ubuntu.com vor...@debian.org


signature.asc
Description: PGP signature

Re: DEB_BUILD_OPTIONS=nowerror

[Sam Hartman]

> "Steve" == Steve Langasek  writes:
Steve> If this is for people doing out-of-archive builds who don't
Steve> want to deal with failures from -Werror, I can see how having
Steve> a single environment toggle is useful to them; but it doesn't
Steve> seem useful to *Debian* when such out-of-archive rebuilds
Steve> don't correspond to the official builds.  E.g. if they're
Steve> test builds for new toolchains, knowing that the package
Steve> *could* build, with options Debian doesn't actually use, is
Steve> of limited use.  (If you build twice, once with once without
Steve> and file bugs on packages where the results differ, I guess
Steve> that's one use, but involves a lot of manual labor.)

In the general case I agree.
But we have the specific case of cross-bootstrapping.
They want to be able to do builds to bootstrap and they want to have an
option they can pass into the build to ask  debian/rules not to include
-Werror.

I suspect Helmut believes he can get patches accepted in the packages
where this is most important to honor the option.
Given his track record, I bet he's right.

So, we have a team in Debian wanting a interface sufficiently
standardized for them to do their work.
I think we have confidence that once we agree on a interface they can
produce appropriate consumers of the interface as well as
implementations of the interface.

I think we should have a high bar for standing in the way of this kind
of work.


signature.asc
Description: PGP signature

Re: DEB_BUILD_OPTIONS=nowerror

[Steve Langasek]

On Mon, Feb 27, 2023 at 10:49:48AM -0700, Sam Hartman wrote:

> Helmut> The problem here specifically arises, because we do not have
> Helmut> consensus on -Werror being a bad idea in release builds.

> I agree with your reading of consensus and for that reason support
> registering an option to say do not use -Werror.

Sorry if I've missed it in this thread, but who is this option supposed to
be *for*?

Generally speaking, DEB_BUILD_OPTIONS or DEB_BUILD_MAINT_OPTIONS flags are
useful for permuting the output of dpkg-buildflags, when the default output
of that command is unsuitable for a particular package or for a particular
rebuild environment.  But here, the output of dpkg-buildflags is not
incorrect because it emits neither -Werror nor -Wno-error; we are talking
about an interface to override upstream defaults, not Debian defaults.

If this is for the maintainer, it seems an unnecessary indirection to define
a flag for the maintainer to set, to configure dpkg-buildflags, to output
the correct flag that you know you want to pass to your package's build
system.  (Maybe the reasoning here is that it's a simpler interface for
maintainers that abstracts a lot of the reasoning about build systems and
makes for a more compact expression in debian/rules?  I'm not sure.)

If this is for people doing out-of-archive builds who don't want to deal
with failures from -Werror, I can see how having a single environment toggle
is useful to them; but it doesn't seem useful to *Debian* when such
out-of-archive rebuilds don't correspond to the official builds.  E.g. if
they're test builds for new toolchains, knowing that the package *could*
build, with options Debian doesn't actually use, is of limited use.  (If you
build twice, once with once without and file bugs on packages where the
results differ, I guess that's one use, but involves a lot of manual labor.)


My understanding is that the argument here is that -Werror is not a sensible
option to use for production builds, that it should only be used for
upstream test builds.  I'm not convinced this is true; I know we've more
than once caught programming errors of various severities downstream by
building on ports that upstreams would generally not be in a position to
cover in their developer test builds.  But if that's the argument, then I
think the logical desired policy outcome would be for dpkg-buildflags to be
changed to emit -Wno-error *by default*, with an option flag along the lines
of DEB_BUILD_OPTIONS=Werror that lets you turn it on instead.

-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developer   https://www.debian.org/
slanga...@ubuntu.com vor...@debian.org


signature.asc
Description: PGP signature

Re: DEB_BUILD_OPTIONS=nowerror

[Sam Hartman]

Helmut> The problem here specifically arises, because we do not have
Helmut> consensus on -Werror being a bad idea in release builds.

I agree with your reading of consensus and for that reason support
registering an option to say do not use -Werror.

Re: DEB_BUILD_OPTIONS=nowerror

[Adrian Bunk]

On Sun, Feb 26, 2023 at 08:25:25PM +0100, Helmut Grohne wrote:
> On Sun, Feb 26, 2023 at 07:15:45PM +0200, Adrian Bunk wrote:
> > What you describe is an RC bug as soon as the more recent toolchain
> > becomes default, and the correct solution is to not build with -Werror. 
> >
> > DEB_BUILD_OPTIONS=nowerror would imply that building with -Werror
> > by default would be OK, but there are already too many FTBFS due
> > to -Werror.
> 
> I would happily agree with all of this, but I do not see consensus on
> either.

My point is that an opt-out DEB_BUILD_OPTIONS=nowerror would make the 
problem worse for the "FTBFS on buildds" problem, since it would result
in more people building their packages with -Werror by default.

>...
> The problem here specifically arises, because we do not have consensus
> on -Werror being a bad idea in release builds.
>...

Strictly disallowing all usage of -Werror (which might be set by the 
maintainer, but more often by upstream) would be controversial.

It would also be hard to define what exactly would be forbidden.
Individual warnings can be turned into errors, and our hardening
flags set -Werror=format-security.

There might be more consensus for language in Policy discouraging
-Werror that leaves maintainers room to diverge from the recommendation?

> Helmut

cu
Adrian

Re: DEB_BUILD_OPTIONS=nowerror

[Helmut Grohne]

On Sun, Feb 26, 2023 at 07:15:45PM +0200, Adrian Bunk wrote:
> What you describe is an RC bug as soon as the more recent toolchain
> becomes default, and the correct solution is to not build with -Werror. 
>
> DEB_BUILD_OPTIONS=nowerror would imply that building with -Werror
> by default would be OK, but there are already too many FTBFS due
> to -Werror.


I would happily agree with all of this, but I do not see consensus on
either.

> DEB_BUILD_OPTIONS=werror as standard name for an opt-in option for CI 
> builds would be a better solution.

Why would I disagree? But that's not the problem I'm trying to solve.

> >...
> > Examples:
> > * glibc adds -Werror
> >...
> 
> glibc does not use the default gcc, which avoids most of the problems 
> you are worried about (but is not a general solution).

In a bootstrap build, glibc must be built with the toolchain that
provides libgcc-s1, so building glibc with a newer toolchain is
unavoidable. The selection of a specific toolchain is not a solution to
the -Werror problem and exactly why we are discussing it.

If glibc and nss were to drop -Werror (which was tried before), I
wouldn't be asking for an option.

The problem here specifically arises, because we do not have consensus
on -Werror being a bad idea in release builds.

However, maybe I can ask Aurelien whether we can opt out of -Werror
whenever a user specifies DEB_GCC_VERSION.

Helmut

Re: DEB_BUILD_OPTIONS=nowerror

[Adrian Bunk]

On Fri, Feb 24, 2023 at 07:19:41AM +0100, Helmut Grohne wrote:
>...
>  * A package adds -Werror to the build. When a new toolchain version is
>uploaded, it triggers a new warning and that makes the package FTBFS.
>...
> When building affected packages with more recent toolchains, such build
> failures are annoying. In a bootstrap setting, they hide later problems.
> For that reason, I would like to have a standard way to opt out of such
> failures. I understand that some of the warnings may be pointing at real
> bugs and that ignoring them certainly is a compromise. I also understand
> that packages may fail to build for other reasons with new toolchains
> (e.g. missing #includes). However, -Werror has proven to be quite
> repetitive and seems worthwhile to address to me.
> 
> As such, I propose a generic DEB_BUILD_OPTIONS=nowerror modelled after
> the original observation,
>...
> So let me know if you think this is a bad idea.

What you describe is an RC bug as soon as the more recent toolchain
becomes default, and the correct solution is to not build with -Werror. 

DEB_BUILD_OPTIONS=nowerror would imply that building with -Werror
by default would be OK, but there are already too many FTBFS due
to -Werror.

DEB_BUILD_OPTIONS=werror as standard name for an opt-in option for CI 
builds would be a better solution.

>...
> Examples:
> * glibc adds -Werror
>...

glibc does not use the default gcc, which avoids most of the problems 
you are worried about (but is not a general solution).

> Helmut

cu
Adrian

Re: DEB_BUILD_OPTIONS=nowerror

[Simon McVittie]

On Fri, 24 Feb 2023 at 09:14:15 -0800, Russ Allbery wrote:
> Simon McVittie  writes:
> > In a typical build system like Autotools, CMake or Meson, it's going to
> > be much, much easier for the answer to be yes, because the obvious way
> > to make linters easy to run is to implement them as a (slightly
> > specialized) test.
> 
> I agree for separate linters, but I'm not sure this is true for -Werror.

Oh, yes, absolutely - I was thinking only of tools like shellcheck here,
not -Werror. Yes, I agree that if -Werror is in-scope for a
DEB_BUILD_OPTIONS flag (whether it's called nolint or nofatalwarnings or
whatever), it wouldn't make much sense for nocheck to disable -Werror.

I think if we say that nocheck MAY disable some or all lint checks,
that covers everything? (It will often disable shellcheck while not
affecting -Werror, but a MAY allows that).

smcv

Re: DEB_BUILD_OPTIONS=nowerror

[Russ Allbery]

Simon McVittie  writes:

> In a typical build system like Autotools, CMake or Meson, it's going to
> be much, much easier for the answer to be yes, because the obvious way
> to make linters easy to run is to implement them as a (slightly
> specialized) test.

I agree for separate linters, but I'm not sure this is true for -Werror.
It's an *interesting* way of handling compiler linter flags that would
have a lot of advantages, but it's a bit annoying to set up.  I think it's
more common for projects to decide what compiler flags to use in Autoconf
or to have a separate target to build with -Werror enabled (or just enable
it unconditionally, alas).

> The way I've generally set up lint checks in my recent projects is to
> make them a test that usually always passes (with non-fatal warnings
> when a problem is detected, like "not ok # TODO" in TAP syntax), and
> then have a non-default way to turn those warnings into a test failure,
> which I use in upstream CI (but usually not in Debian packaging).

A bit of an aside, but I use the Lancaster consensus environment variables
for this in all of my projects, which came out of the Perl community but
aren't Perl-specific.

https://github.com/Perl-Toolchain-Gang/toolchain-site/blob/master/lancaster-consensus.md#environment-variables-for-testing-contexts

RELEASE_TESTING and AUTHOR_TESTING should not be run during normal builds,
only during the QA process during development and release preparation.
(although sometimes I turn on RELEASE_TESTING in the Debian build if I
know the chances of it failing are very low).  AUTOMATED_TESTING would
generally be appropriate for running during a Debian package build.
(Usually that is for tests that require a bunch of prerequisites that
aren't needed to build the software, only to test it.)

Now that we have autopkgtests, I probably should disable RELEASE_TESTING
in all cases during the regular package build and enaable it only for
autopkgtests, but I haven't done that.

It would be great if more upstreams would pick up these conventions.  The
Perl community already has some tools to deal with them, and I have
additional Perl modules in my gnulib-like collection of helpers that I
copy into my packages.

-- 
Russ Allbery (r...@debian.org)  

Re: DEB_BUILD_OPTIONS=nowerror

[Andrey Rakhmatullin]

On Fri, Feb 24, 2023 at 08:27:53AM +0100, Helmut Grohne wrote:
> > Also I think it was recommended to *not* use -Werror by default as it
> > is too fragile. Maybe one should have a "developer mode" flag instead
> > that allows using -Werror?
> 
> Well, if we were avoiding -Werror by default, we wouldn't have this
> discussion. It certainly isn't consensus.
It was also my understanding that we recommend not using -Werror during
build process and maybe even not enabling it by default by upstreams. I
don't know if it's written down anywhere, maybe it should be.
Also if something is not implemented in many packages it doesn't mean it's
not the project consensus, maybe it's just not enforced and not
recommended enough yet :)

Re: DEB_BUILD_OPTIONS=nowerror

[Simon McVittie]

On Fri, 24 Feb 2023 at 12:11:19 +0100, Helmut Grohne wrote:
> On Fri, Feb 24, 2023 at 10:58:37AM +0100, Johannes Schauer Marin Rodrigues 
> wrote:
> > Should other linters like shellcheck be disabled with
> > DEB_BUILD_OPTIONS=nocheck?
> 
> I argue for "no" (see above).

In a typical build system like Autotools, CMake or Meson, it's going to
be much, much easier for the answer to be yes, because the obvious way to
make linters easy to run is to implement them as a (slightly specialized)
test. I think a pragmatic approach for Debian would be to say that nolint
SHOULD either disable lint checks, and nocheck MAY disable lint checks.

The way I've generally set up lint checks in my recent projects is to
make them a test that usually always passes (with non-fatal warnings
when a problem is detected, like "not ok # TODO" in TAP syntax), and
then have a non-default way to turn those warnings into a test failure,
which I use in upstream CI (but usually not in Debian packaging). I
think doing the lint checks, but ignoring their results other than as
human-readable diagnostics in the build log, should be considered a
valid implementation of nolint.

smcv

Re: DEB_BUILD_OPTIONS=nowerror

[Helmut Grohne]

On Fri, Feb 24, 2023 at 10:58:37AM +0100, Johannes Schauer Marin Rodrigues 
wrote:
> I question the conflation of a hypothetical DEB_BUILD_OPTIONS=nowerror with
> other linters like shellcheck (or other tools for other programming 
> languages).

I didn't quite reason about that aspect and now see how it is not
obvious.

> I'm surprised you are conflating shellcheck with the question about -Werror. I
> don't yet understand why you would like to skip shellcheck by setting
> DEB_BUILD_OPTIONS=nowerror and not by setting DEB_BUILD_OPTIONS=nocheck?

I see -Werror and shellcheck as something fundamentally different to
tests, but maybe that view is not universal.

 * Tests exist to verify that implemented functionality works as
   intended. Tests can fail when the environment changes, but when they
   fail, typically a practical use case also breaks. With -Werror,
   shellcheck, and black, we often have harmless warnings, false positives
   or bugs that are difficult to trigger. So linter violations tend to not
   impact the utility of a software as badly.
 * In a cross build, we cannot run tests, but we can run linters. That
   is a small reason to keep these concepts separate.

Proposed distinction: Tests highlight actual problems by executing the
implementation and linters highlight possible problems by analyzing the
implementation without actually running it.

> Yes, -Werror is also a kind of check but whether -Werror should be not be
> passed when DEB_BUILD_OPTIONS=nocheck is given, is a slightly different
> question I think.

Yeah and for the reasons above, I wouldn't like -Werror to be disabled
by nocheck.

> Quoting Shengjing Zhu (2023-02-24 09:14:46)
> > IMO, these are just linters. And shouldn't not run after the source is 
> > released.

I think this quite nails it. Thank you for raising the term "linter".
And yeah, the common wisdom is to run them on CI, but not during package
builds except for some packages that enable them in package builds as
well.

> > I'd like to propose the option name `nolint`.

This fully addresses the concerns raised by Ansgar I think. I like it.
Thank you.

> I think that there is value to run linters at build time in a default build
> because (as shown by Helmut's initial mail) new version of linters may show
> different problems compared to the old version and those only get fixed if the
> build breaks. So I think it's useful to have a package FTBFS if a new version
> of a linter introduces a failure.

Of course there is value in running them! But there also is a cost.
Quite clearly, we do not have distribution-wide consensus on that
trade-off. And the difference is subtle: Those who think that a default
build shouldn't run linters tend to also think that CI should run
linters. So it's not about disabling linters, but about keeping packages
buildable and not introducing latent build failures.

> But of course there is also a use-case of disabling linters if one is not 
> doing
> a default build. So I think the more interesting questions are:
> 
> Should there be a new DEB_BUILD_OPTIONS=nowerror that disables -Werror?

Yeah, that's the question I raised effectively. (+ painting it
correctly)

> Should DEB_BUILD_OPTIONS=nowerror also disable other linters?

I still argue for "yes" and "nolint" makes more sense of that.

> Should other linters like shellcheck be disabled with
> DEB_BUILD_OPTIONS=nocheck?

I argue for "no" (see above).

> Should -Werror be disabled with DEB_BUILD_OPTIONS=nocheck?

I argue for "no" (see above).

I gather that things are not as obvious as I thought and that asking
d-devel was a good thing before sending patches. :)

Helmut

Re: DEB_BUILD_OPTIONS=nowerror

[Simon McVittie]

On Fri, 24 Feb 2023 at 10:58:37 +0100, Johannes Schauer Marin Rodrigues wrote:
> I question the conflation of a hypothetical DEB_BUILD_OPTIONS=nowerror with
> other linters like shellcheck (or other tools for other programming 
> languages).

Compiler warnings and lint tools do share some properties:

* they often indicate a bug, but can easily have false positives
* upgrading the "toolchain" (the compiler or the lint tool) frequently
  improves its ability to detect potential bugs, but can also detect new
  false positives
* if they indicate a bug, the bug is not necessarily reachable in practice
* if a "toolchain" upgrade indicates a new bug, the bug is reasonably
  likely to be non-RC

so I think it probably does make sense to think about them in a similar
way.

Compare with a typical functional test (unit test or similar), where a
failure indicates that something is observably wrong with the software
under test - not just a potential bug, but something that the author of
the test has decided is sufficiently wrong behaviour to fail the test,
as detected by a test that is maintained alongside the package itself.

If lint tools and compiler warnings are gated by the same option, I'd
call it (no)fatalwarnings, and I think there is rough consensus (but not
necessarily 100%) that making compiler warnings and lint warnings fatal
during a released package build is the wrong default. Something I've
sometimes done is to gate this sort of thing on whether debian/changelog
says UNRELEASED (=> fatal warnings) or something else (=> non-fatal
warnings).

smcv

Re: DEB_BUILD_OPTIONS=nowerror

[Johannes Schauer Marin Rodrigues]

Quoting Helmut Grohne (2023-02-24 08:27:53)
> shellcheck:
>  * grml-debootstrap

not run with DEB_BUILD_OPTIONS=nocheck
https://sources.debian.org/src/grml-debootstrap/0.103/debian/rules/?hl=13#L13

>  * josm-installer

not run with DEB_BUILD_OPTIONS=nocheck
https://sources.debian.org/src/josm-installer/0.0.2+svn18515/debian/rules/?hl=25#L25

>  * kdump-tools

not run with DEB_BUILD_OPTIONS=nocheck
https://sources.debian.org/src/kdump-tools/1:1.8.1/debian/rules/?hl=64#L64

>  * python-sshoot

not run with DEB_BUILD_OPTIONS=nocheck (as dh_auto_test is skipped in that case)
https://sources.debian.org/src/python-sshoot/1.4.2-2/debian/rules/?hl=23#L23

>  * sshcommand

Indeed, that one seems to always run shellcheck:
https://sources.debian.org/src/sshcommand/0~20160110.1~2795f65-1.1/debian/rules/?hl=7#L7

>  * uwsgi

Also always run:
https://sources.debian.org/src/uwsgi/2.0.21-4/debian/rules/?hl=570#L570


I question the conflation of a hypothetical DEB_BUILD_OPTIONS=nowerror with
other linters like shellcheck (or other tools for other programming languages).

I believe that sshcommand and uwsgi should have shellcheck not run with
DEB_BUILD_OPTIONS=nocheck, so I filed:

https://salsa.debian.org/debian/sshcommand/-/merge_requests/1
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1031857

I'm surprised you are conflating shellcheck with the question about -Werror. I
don't yet understand why you would like to skip shellcheck by setting
DEB_BUILD_OPTIONS=nowerror and not by setting DEB_BUILD_OPTIONS=nocheck?

Yes, -Werror is also a kind of check but whether -Werror should be not be
passed when DEB_BUILD_OPTIONS=nocheck is given, is a slightly different
question I think.

Quoting Shengjing Zhu (2023-02-24 09:14:46)
> On Fri, Feb 24, 2023 at 2:26 PM Helmut Grohne  wrote:
> >
> > Hi,
> >
> > I observe a pattern repeated at least twice and probably more often in
> > packages.
> >
> >  * A package adds -Werror to the build. When a new toolchain version is
> >uploaded, it triggers a new warning and that makes the package FTBFS.
> >  * A package runs shellcheck during build. When a new shellcheck is
> >uploaded, it triggers a new warning and makes the package FTBFS.
> >
>█
> IMO, these are just linters. And shouldn't not run after the source is 
> released.
>█
> I'd like to propose the option name `nolint`.

I think that there is value to run linters at build time in a default build
because (as shown by Helmut's initial mail) new version of linters may show
different problems compared to the old version and those only get fixed if the
build breaks. So I think it's useful to have a package FTBFS if a new version
of a linter introduces a failure.

But of course there is also a use-case of disabling linters if one is not doing
a default build. So I think the more interesting questions are:

Should there be a new DEB_BUILD_OPTIONS=nowerror that disables -Werror?

Should DEB_BUILD_OPTIONS=nowerror also disable other linters?

Should other linters like shellcheck be disabled with
DEB_BUILD_OPTIONS=nocheck?

Should -Werror be disabled with DEB_BUILD_OPTIONS=nocheck?

Thanks!

cheers, josch

signature.asc
Description: signature

Re: DEB_BUILD_OPTIONS=nowerror

[Shengjing Zhu]

On Fri, Feb 24, 2023 at 2:26 PM Helmut Grohne  wrote:
>
> Hi,
>
> I observe a pattern repeated at least twice and probably more often in
> packages.
>
>  * A package adds -Werror to the build. When a new toolchain version is
>uploaded, it triggers a new warning and that makes the package FTBFS.
>  * A package runs shellcheck during build. When a new shellcheck is
>uploaded, it triggers a new warning and makes the package FTBFS.
>

IMO, these are just linters. And shouldn't not run after the source is released.

I'd like to propose the option name `nolint`.

-- 
Shengjing Zhu

Re: DEB_BUILD_OPTIONS=nowerror

[Helmut Grohne]

Hi Ansgar,

On Fri, Feb 24, 2023 at 08:02:35AM +0100, Ansgar wrote:
> The name is too specific and can be misread:
> 
> - It is specific to -Werror, but other similar systems exist.
> 
> - It can be easily read an "now error" (i.e., a warning
>   should "now (be an) error").

Fair point. I am unimagintaive about better terms at present.

> Also I think it was recommended to *not* use -Werror by default as it
> is too fragile. Maybe one should have a "developer mode" flag instead
> that allows using -Werror?

Well, if we were avoiding -Werror by default, we wouldn't have this
discussion. It certainly isn't consensus.

-Werror:
 * acpid
 * blktrace
 * bomstrip
 * breeze-plymouth
 * cbmc
 * cryptsetup
 * diagnostics
 * dumpet
 * glibc
 * golang-gvisor-gvisor
 * libgadu
 * libutempter
 * nss
 * quotatool
 * smcroute
 * spiped
 * switchsh
 * varnish

shellcheck:
 * grml-debootstrap
 * josm-installer
 * kdump-tools
 * python-sshoot
 * sshcommand
 * uwsgi

Admittedly, we also have a fair number of packages that explicitly
disable -Werror, so we also don't have consensus on applying -Werror.

Given this non-consistency, I'm asking for a way to opt out, but I'd
also be happy if this really was off by default and interested people
would opt in. This does have a precedent with DEB_BUILD_OPTIONS=terse
where we effectively settled on verbose by default.

Helmut

Re: DEB_BUILD_OPTIONS=nowerror

[Johannes Schauer Marin Rodrigues]

Quoting Helmut Grohne (2023-02-24 07:19:41)
> As such, I propose a generic DEB_BUILD_OPTIONS=nowerror modelled after the
> original observation, but meant to also match other checkers such as
> shellcheck. The general idea should be that a warning should that can be
> non-fatal should be non-fatal if possible.

Are there packages that run shellcheck in other places than those disabled by
DEB_BUILD_OPTIONS=nocheck? If yes, should running shellcheck not be moved to
there? In contrast to -Werror, shellcheck is not involved in literally building
something.

Otherwise, other linter tools like black come to mind that also recently broke
my packages [1] when black got upgraded to 23.1.0-1 two weeks ago. But black
(like shellcheck I'd assume) is usually disabled with nocheck or by not running
autopkgtests.

In what places is shellcheck called such that it cannot or should not be moved
to a place where it could be disabled by DEB_BUILD_OPTIONS=nocheck?

Thanks!

cheers, josch

[1] Versions of mmdebstrap and diffoscope that work with the new black in their
autopkgtest have already been uploaded to unstable and are waiting to
transition to testing so that black can transition as well. Maybe the timing of
this upload of black was a bit unfortunate this late into the freeze because
other packages are using it as part of their build process and now FTBFS. Like
another package of mine (botch) which is also fixed now and waiting:
https://bugs.debian.org/1031469

Re: DEB_BUILD_OPTIONS=nowerror

[Ansgar]

On Fri, 2023-02-24 at 07:19 +0100, Helmut Grohne wrote:
> As such, I propose a generic DEB_BUILD_OPTIONS=nowerror modelled after
> the original observation, but meant to also match other checkers such as
> shellcheck. The general idea should be that a warning should that can be
> non-fatal should be non-fatal if possible.

The name is too specific and can be misread:

- It is specific to -Werror, but other similar systems exist.

- It can be easily read an "now error" (i.e., a warning
  should "now (be an) error").

Also I think it was recommended to *not* use -Werror by default as it
is too fragile. Maybe one should have a "developer mode" flag instead
that allows using -Werror?

Ansgar

DEB_BUILD_OPTIONS=nowerror

[Helmut Grohne]

Hi,

I observe a pattern repeated at least twice and probably more often in
packages.

 * A package adds -Werror to the build. When a new toolchain version is
   uploaded, it triggers a new warning and that makes the package FTBFS.
 * A package runs shellcheck during build. When a new shellcheck is
   uploaded, it triggers a new warning and makes the package FTBFS.

In general, the vast majority of packages does not use -Werror (except
specifically, e.g. via dpkg-buildflags) or shellcheck during build, but
employs such techniques during e.g. salsa-ci and that seems generally
preferred. However, some packages do still use this pattern.

Examples:
 * glibc adds -Werror
 * nss adds -Werror

When building affected packages with more recent toolchains, such build
failures are annoying. In a bootstrap setting, they hide later problems.
For that reason, I would like to have a standard way to opt out of such
failures. I understand that some of the warnings may be pointing at real
bugs and that ignoring them certainly is a compromise. I also understand
that packages may fail to build for other reasons with new toolchains
(e.g. missing #includes). However, -Werror has proven to be quite
repetitive and seems worthwhile to address to me.

As such, I propose a generic DEB_BUILD_OPTIONS=nowerror modelled after
the original observation, but meant to also match other checkers such as
shellcheck. The general idea should be that a warning should that can be
non-fatal should be non-fatal if possible.

Does this proposal make sense? Is there sufficient use case to support
it as a generic distribution feature (for a niche number of packages)?

>From a process point of view, I think this mail serves as a possible
standardization point. Packages can add support for this and the
responsibility for providing patches for this generally resides with
those interested in having it (i.e. me sending patches). Once we have
some adoption of this, we can add it to Debian policy.

So let me know if you think this is a bad idea.

Helmut