Re: Do not use -Werror by default in your modules without joining #testable

[Emmanuele Bassi]

On 3 April 2017 at 19:38, Nicolas Dufresne  wrote:
> Le dimanche 02 avril 2017 à 14:59 +0100, Emmanuele Bassi a écrit :
>> Yes, I know: this would be slightly more easier if Continuous warned
>> about build breakages via email (though I'm pretty sure email would
>> still be a high latency medium that tends to be ignored);
>> nevertheless, joining the #testable IRC channel *today* to get a
>> notification of build failure is *not* a heavy burden — especially
>> now
>> that we have the Matrix bridge and you don't even need an IRC client
>> running at all times.
>
> Is this important for projects that already have their own CI system ?
> (notably GStreamer). It does seems like an overhead to track two CI, I
> do believe that build breakage due to warning should be quite rare in
> GStreamer case. Please let us know.

If you have your own CI then, by all means: keep watching your own CI. :-)

GStreamer is fairly well-behaved, and it's rare that I have to hunt
down people on IRC, or file a bug for it.

Ciao,
 Emmanuele.

-- 
https://www.bassi.io
[@] ebassi [@gmail.com]
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Nicolas Dufresne]

Le dimanche 02 avril 2017 à 14:59 +0100, Emmanuele Bassi a écrit :
> Yes, I know: this would be slightly more easier if Continuous warned
> about build breakages via email (though I'm pretty sure email would
> still be a high latency medium that tends to be ignored);
> nevertheless, joining the #testable IRC channel *today* to get a
> notification of build failure is *not* a heavy burden — especially
> now
> that we have the Matrix bridge and you don't even need an IRC client
> running at all times.

Is this important for projects that already have their own CI system ?
(notably GStreamer). It does seems like an overhead to track two CI, I
do believe that build breakage due to warning should be quite rare in
GStreamer case. Please let us know.

Nicolas 


signature.asc
Description: This is a digitally signed message part
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Michael Catanzaro]

On Sun, 2017-04-02 at 20:22 +0200, Sébastien Wilmet wrote:
> To disable -Werror by default it's better to set the IS-RELEASE
> parameter of AX_COMPILER_FLAGS to "yes":
> 
> AX_COMPILER_FLAGS([WARN_CFLAGS], [WARN_LDFLAGS], [yes])

Yeah I think you're right, that's what Emmanuele wants... OTOH, this
seems almost like abuse of the macro.

Michael
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Michael Catanzaro]

On Mon, 2017-04-03 at 06:42 +0200, Sébastien Wilmet wrote:
> Maybe this works:
> module_autogenargs['epiphany'] = '--enable-Werror'

Actually it does work. No clue what I was doing wrong when I tested
this previously. :)

module_autogenargs['epiphany'] = 'CFLAGS="-Werror"' works too. I didn't
try messing with makeargs as that doesn't seem right.

Michael
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Sébastien Wilmet]

On Sun, Apr 02, 2017 at 06:22:07PM +0100, Emmanuele Bassi wrote:
> On 2 April 2017 at 18:16, Michael Catanzaro  wrote:
> > On Sun, 2017-04-02 at 15:59 +0100, Emmanuele Bassi wrote:
> >> Adding:
> >>
> >>   disable_Werror = False
> >>
> >> to your jhbuildrc should do the trick.
> >
> > Ah, but nobody wants to do that for all modules locally... I only want
> > it for Epiphany.
> 
> Adding this:
> 
> ```
> module_autogenargs['epiphany'] = '--disable-Werror'
> ```
> 
> should work, assuming Epiphany uses the m4 macros that add `--disable-Werror`.

So, --disable-Werror for all modules, except epiphany.

Maybe this works:
module_autogenargs['epiphany'] = '--enable-Werror'

but if the command line contains both --disable-Werror (because of
Jhbuild defaults) and --enable-Werror, I don't know how it'll behave.

Or this:
module_makeargs['epiphany'] = 'CFLAGS="-Werror"'

?

--
Sébastien
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Michael Catanzaro]

On Sun, 2017-04-02 at 18:22 +0100, Emmanuele Bassi wrote:
> Adding this:
> 
> ```
> module_autogenargs['epiphany'] = '--disable-Werror'
> ```
> 
> should work, assuming Epiphany uses the m4 macros that add `
> --disable-Werror`.

It doesn't work unfortunately. I don't remember why but it didn't look
easy to fix last I was fighting this.

Michael
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Emmanuele Bassi]

On 2 April 2017 at 19:20,   wrote:

> My understanding was the objection was against using Werror as the default
> while _not_ paying attention to the CI notifications. Please correct me if
> I've misunderstood this.

That is correct.

If the goal of:

 - enabling -Werror by default when building from Git
 - passing --disable-Werror by default for new jhbuild users

is relying on a CI/CD pipeline to proactively fix build issues caught
by -Werror, then people opting into this by using autotools macros
should *really* be looking at the Continuous pipeline state. We have
old school "push notifications" in the form of the #testable IRC
channel (and the build state gets also relayed to #gnome-hackers on
success/failure transitions).

If you don't care about this kind of things then, by all means:
disable errors by default in your modules.

Ciao,
 Emmanuele.

-- 
https://www.bassi.io
[@] ebassi [@gmail.com]
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Sébastien Wilmet]

On Sun, Apr 02, 2017 at 07:21:38PM +0200, Sébastien Wilmet wrote:
> 1) This warning: comparison between signed and unsigned integer.

After a bit more thoughts, I see one flaw in my reasoning, because GCC
triggers the above warning depending on the architecture it is building
for. When the ARM CI server was set up for GNOME, someone filed a bug
about this warning, but on x86_64 the warning was not present.

I consider that a bug in GCC, it should show warnings depending on the C
standard, not depending on a specific architecture.

--
Sébastien
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Sébastien Wilmet]

On Sun, Apr 02, 2017 at 09:53:41AM -0500, Michael Catanzaro wrote:
> Simplest solution would be for Continuous to just pass --disable-Werror 
> to configure. OTOH I'm willing to change the Epiphany behavior if you
> don't agree, but it's baked into use of AX_IS_RELEASE([git-directory])
> and AX_COMPILER_FLAGS, which to my understanding is the recommended set
> of macros to use for GNOME modules. So we need to adjust that if we
> want to change this. The behavior you want would require using
> AX_IS_RELEASE([always]), which doesn't seem great to me. I think that's
> the only way to get the behavior you want without totally dropping use
> of AX_COMPILER_FLAGS, which we probably don't want to do. (Am I
> mistaken?)

AX_IS_RELEASE([always]) might have unintended effects for other macros
than AX_COMPILER_FLAGS.

To disable -Werror by default it's better to set the IS-RELEASE
parameter of AX_COMPILER_FLAGS to "yes":

AX_COMPILER_FLAGS([WARN_CFLAGS], [WARN_LDFLAGS], [yes])

I think that's what I'll do in the modules I maintain, because I don't
use -Werror, and there can be deprecation warnings that pop up at any
time. -Werror is anyway disabled by default in Jhbuild, so I don't see
why it should be enabled by default for other people not using Jhbuild
and want to build the module from git.

--
Sébastien
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[philip . chimento]

On Sun, Apr 2, 2017, 10:21 Sébastien Wilmet  wrote:

> On Sun, Apr 02, 2017 at 04:52:49PM +0100, Emmanuele Bassi wrote:
> > You seem to misunderstand what a continuous delivery/continuous
> > integration pipeline is for.
> 
>
> I think I understand what a CI server is for, I simply disagree with
> you.
>
> Let's compare two scenarios:
>
> 1) This warning: comparison between signed and unsigned integer.
> 2) A real build error due to a change in an underlying library.
>
> For the sake of argument, 1) can be replaced by any warning that becomes
> an error if -Werror is enabled. I.e. not a "real" build failure, by
> default it's just a warning.
>
> In most cases, if 1) appears, the problem is located in the code of the
> module itself, it's not caused by a dependency. So the developer
> directly sees it when building the module in jhbuild, even if the deps
> are not up-to-date.
>
> For 2), it's better that it is detected by a CI server so that we know
> the problem as soon as possible.
>
> A lot of GNOME modules have compilation warnings, and I don't consider
> them critically important. In fact, -Werror is disabled in tarballs
> (what we actually ship to distros). Of course it's better to fix them,
> and in the modules that I maintain they are all fixed except deprecation
> warnings. I won't push a commit on the master branch if it adds a
> warning, because I directly see the warning when building the code
> locally.
>
> On the other hand it's nice that the CI server detects 2) because it's
> not practical to rebuild the dependencies in jhbuild all the time.
>

I think you might still be misunderstanding what Emmanuele is asking.

If you don't believe that the CI server should detect case (1), or you
prefer to have your own watchfulness as the last line of defense, then
simply don't enable Werror by default on your module. Use a configuration
locally that allows you to catch the warnings you want before you push, and
there's no need to bother with the CI server.

If you _do_ believe that the CI server should detect case (1), then make
Werror the default on your module, and watch the CI server notifications in
case some warning slips past you when you push, so that other people are
not inconvenienced.

My understanding was the objection was against using Werror as the default
while _not_ paying attention to the CI notifications. Please correct me if
I've misunderstood this.

Regards,
Philip C

>
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Emmanuele Bassi]

On 2 April 2017 at 18:16, Michael Catanzaro  wrote:
> On Sun, 2017-04-02 at 15:59 +0100, Emmanuele Bassi wrote:
>> Adding:
>>
>>   disable_Werror = False
>>
>> to your jhbuildrc should do the trick.
>
> Ah, but nobody wants to do that for all modules locally... I only want
> it for Epiphany.

Adding this:

```
module_autogenargs['epiphany'] = '--disable-Werror'
```

should work, assuming Epiphany uses the m4 macros that add `--disable-Werror`.

Maybe jhbuild could have a:

```
module_disable_Werror['epiphany'] = True
```

but at this point we're sliding into diminishing returns, as it
doesn't buy you much.

Ciao,
 Emmanuele.

-- 
https://www.bassi.io
[@] ebassi [@gmail.com]
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Sébastien Wilmet]

On Sun, Apr 02, 2017 at 04:52:49PM +0100, Emmanuele Bassi wrote:
> You seem to misunderstand what a continuous delivery/continuous
> integration pipeline is for.


I think I understand what a CI server is for, I simply disagree with
you.

Let's compare two scenarios:

1) This warning: comparison between signed and unsigned integer.
2) A real build error due to a change in an underlying library.

For the sake of argument, 1) can be replaced by any warning that becomes
an error if -Werror is enabled. I.e. not a "real" build failure, by
default it's just a warning.

In most cases, if 1) appears, the problem is located in the code of the
module itself, it's not caused by a dependency. So the developer
directly sees it when building the module in jhbuild, even if the deps
are not up-to-date.

For 2), it's better that it is detected by a CI server so that we know
the problem as soon as possible.

A lot of GNOME modules have compilation warnings, and I don't consider
them critically important. In fact, -Werror is disabled in tarballs
(what we actually ship to distros). Of course it's better to fix them,
and in the modules that I maintain they are all fixed except deprecation
warnings. I won't push a commit on the master branch if it adds a
warning, because I directly see the warning when building the code
locally.

On the other hand it's nice that the CI server detects 2) because it's
not practical to rebuild the dependencies in jhbuild all the time.

--
Sébastien
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Michael Catanzaro]

On Sun, 2017-04-02 at 15:59 +0100, Emmanuele Bassi wrote:
> Adding:
> 
>   disable_Werror = False
> 
> to your jhbuildrc should do the trick.

Ah, but nobody wants to do that for all modules locally... I only want
it for Epiphany.

Michael
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Emmanuele Bassi]

On 2 April 2017 at 16:44, Sébastien Wilmet  wrote:
> On Sun, Apr 02, 2017 at 03:59:39PM +0100, Emmanuele Bassi wrote:
>> The point of my email was, though, that people should be keeping an
>> eye on their modules on the CD pipeline.
>
> It's easier to keep an eye on our local compilation output, and rebuild
> from time to time the dependencies in jhbuild.

Sure, it's "easier" for some people: out of sight, out of mind.

It's also broken, because it relies on humans to do the work of machines.

> GNOME Continuous is useful to detect as early as possible real build
> breakages (a FTBFS *with* --disable-Werror).

> Or determining the culprit
> commit when a unit test starts to fail (and possibly cross-modules, e.g.
> a commit in gtk that makes an app unit test to fail).

You seem to misunderstand what a continuous delivery/continuous
integration pipeline is for.

The whole point of CD/CI is to detect issues early, even before unit
testing fails. Compilation warnings are just a prelude to additional
issues down the line. Otherwise, why using them at all? You can build
with all the compilation warnings turned off already and not care at
all.

If you specifically enable compilation warnings, and then go to the
extra length of enabling -Werror by default, then it means you do care
about them. Caring about them, though, does not stop at the boundary
of your Git repository; that's why we have a continuous delivery
platform: to figure out the impact of changes between modules, and to
ensure that a whole set of modules builds correctly.

Disabling -Werror in Continuous would make *my* life easier — I
wouldn't have to file bugs or tag modules; it would also defeat the
point of having Continuous in the first place.

So, *please*: be proactive, and use the notifications from the
Continuous builder to know if your module is affected, instead of
waiting on a bug to be filed.

Ciao,
 Emmanuele.

-- 
https://www.bassi.io
[@] ebassi [@gmail.com]
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Sébastien Wilmet]

On Sun, Apr 02, 2017 at 03:59:39PM +0100, Emmanuele Bassi wrote:
> The point of my email was, though, that people should be keeping an
> eye on their modules on the CD pipeline.

It's easier to keep an eye on our local compilation output, and rebuild
from time to time the dependencies in jhbuild.

GNOME Continuous is useful to detect as early as possible real build
breakages (a FTBFS *with* --disable-Werror). Or determining the culprit
commit when a unit test starts to fail (and possibly cross-modules, e.g.
a commit in gtk that makes an app unit test to fail).

--
Sébastien
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Emmanuele Bassi]

On 2 April 2017 at 15:53, Michael Catanzaro  wrote:
> On Sun, 2017-04-02 at 14:59 +0100, Emmanuele Bassi wrote:
>> I know that the effort to use -Werror by default is well-intentioned,
>> and ordinarily I'm in favour of maintainers catching errors locally
>> before pushing code to the public repository.
>>
>> In practice, though, this effort has been insufficient, when not
>> misguided. In order to appeal to newcomers, and to avoid pain to new
>> contributors, jhbuild builds with --disable-Werror; again, entirely
>> in
>> favour of this approach. The rationale given for this was that we
>> would catch errors through the Continuous pipeline — especially when
>> these errors are caused by modules early in the dependency chain.
>
> So with Epiphany the desired behavior is:
>
>  * Developers (by default, anyone using a git checkout) get -Werror
>  * Users (by default, anyone using a tarball or JHBuild) don't
>
> I like it, but it's not perfect because in practice all developers use
> JHBuild, and all my attempts to re-enable Werror from jhbuildrc have
> failed.

Adding:

  disable_Werror = False

to your jhbuildrc should do the trick.

> So I think the only people getting -Werror by default are
> people who actually don't want it.
>
> Simplest solution would be for Continuous to just pass --disable-Werror
> to configure.

Which is what I would rather not do at all, because then *nobody* will
actually fix compiler warnings.

I would probably add:

  -Wno-error=deprecated-declaration

to the default flags, especially during development, considering that
deprecations are not an immediate issue to be fixed, but that would
likely have the fallout of nobody fixing deprecation warnings; not
that it matters much: we still have various bits of our platform using
GSimpleAsyncResult instead of GTask, and that has been deprecated for
more than 2 years.

The point of my email was, though, that people should be keeping an
eye on their modules on the CD pipeline.

Ciao,
 Emmanuele.

-- 
https://www.bassi.io
[@] ebassi [@gmail.com]
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list

Re: Do not use -Werror by default in your modules without joining #testable

[Michael Catanzaro]

On Sun, 2017-04-02 at 14:59 +0100, Emmanuele Bassi wrote:
> I know that the effort to use -Werror by default is well-intentioned,
> and ordinarily I'm in favour of maintainers catching errors locally
> before pushing code to the public repository.
> 
> In practice, though, this effort has been insufficient, when not
> misguided. In order to appeal to newcomers, and to avoid pain to new
> contributors, jhbuild builds with --disable-Werror; again, entirely
> in
> favour of this approach. The rationale given for this was that we
> would catch errors through the Continuous pipeline — especially when
> these errors are caused by modules early in the dependency chain.

So with Epiphany the desired behavior is:

 * Developers (by default, anyone using a git checkout) get -Werror
 * Users (by default, anyone using a tarball or JHBuild) don't

I like it, but it's not perfect because in practice all developers use
JHBuild, and all my attempts to re-enable Werror from jhbuildrc have
failed. So I think the only people getting -Werror by default are
people who actually don't want it.

Simplest solution would be for Continuous to just pass --disable-Werror 
to configure. OTOH I'm willing to change the Epiphany behavior if you
don't agree, but it's baked into use of AX_IS_RELEASE([git-directory])
and AX_COMPILER_FLAGS, which to my understanding is the recommended set
of macros to use for GNOME modules. So we need to adjust that if we
want to change this. The behavior you want would require using
AX_IS_RELEASE([always]), which doesn't seem great to me. I think that's
the only way to get the behavior you want without totally dropping use
of AX_COMPILER_FLAGS, which we probably don't want to do. (Am I
mistaken?)

Michael
___
desktop-devel-list mailing list
desktop-devel-list@gnome.org
https://mail.gnome.org/mailman/listinfo/desktop-devel-list