Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

classic Classic list List threaded Threaded
13 messages Options
Rene Engelhard Rene Engelhard
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

Hi,

On Tue, Feb 20, 2018 at 09:22:10PM +0000, Samuel Thibault wrote:

> New commits:
> commit 226697ae27ef451cad404256e83eef88262f16d1
> Author: Samuel Thibault <[hidden email]>
> Date:   Fri Feb 16 13:22:10 2018 +0100
>
>     Integrate initial version of gla11y tool in the build system
>    
>     This is part of integrating an accessibility non-regression tool. This
>     adds checks in configure.ac for the presence of python lxml which we will
>     need, and adds support for calling the tool at build time, to check for
>     definite UI errors. For now, that only emits errors for missing or duplicate
>     accessibility relation targets, and senseless relations: a label being
>     mnemonic for several widgets.
>    
>     Change-Id: Idda91b15b9a9e0322d16db33dfac8e03f2aa518c
>     Reviewed-on: https://gerrit.libreoffice.org/49856
>     Tested-by: Jenkins <[hidden email]>
>     Reviewed-by: Thorsten Behrens <[hidden email]>

Was this ever really tested besides Jenkins (no idea with what build
config this was tested...)?

> diff --git a/bin/gla11y b/bin/gla11y
> new file mode 100755
> index 000000000000..d0619133ad0f
> --- /dev/null
> +++ b/bin/gla11y
> @@ -0,0 +1,216 @@
> +#!/usr/bin/env python

That's "python". Python2.

[...]

> diff --git a/configure.ac b/configure.ac
> index e20e91e7fa42..479968be94b9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -8148,10 +8148,19 @@ if test $enable_python = system; then
>  fi
>  
>  dnl By now enable_python should be "system", "internal" or "no"
> +PYTHON_LXML=
>  case $enable_python in
>  system)
>      SYSTEM_PYTHON=TRUE
>  
> +    AC_MSG_CHECKING([for python lxml])
> +    if $PYTHON -c "import lxml.etree as ET" ; then
> +        PYTHON_LXML=TRUE
> +        AC_MSG_RESULT([yes])
> +    else
> +        AC_MSG_RESULT([no, will not be able to check UI accessibility])
> +    fi
> +
>      dnl Check if the headers really work
>      save_CPPFLAGS="$CPPFLAGS"
>      CPPFLAGS="$CPPFLAGS $PYTHON_CFLAGS"


Here it checks for lxml in the system python. This is a a 3.x.
Because for python3-uno in Debian, of course python3 is used.
And LOs internal python also is python3.

But the actual script (see above) calls "python" and not "python3"

Even if installing python-xml (for the actual script) and python3
(for the configure check) it complains about no input files or somesuch
and fails....

Regards,

Rene
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

Hello,

Rene Engelhard, on mer. 21 févr. 2018 09:11:02 +0100, wrote:
> Was this ever really tested besides Jenkins (no idea with what build
> config this was tested...)?

I did test it, but apparently not in all potential situations.

> > diff --git a/bin/gla11y b/bin/gla11y
> > new file mode 100755
> > index 000000000000..d0619133ad0f
> > --- /dev/null
> > +++ b/bin/gla11y
> > @@ -0,0 +1,216 @@
> > +#!/usr/bin/env python
>
> That's "python". Python2.

It works with either python2 or python3

> > +    AC_MSG_CHECKING([for python lxml])
> > +    if $PYTHON -c "import lxml.etree as ET" ; then
>
> Here it checks for lxml in the system python. This is a a 3.x.
> Because for python3-uno in Debian, of course python3 is used.
> And LOs internal python also is python3.
>
> But the actual script (see above) calls "python" and not "python3"

Mmm, so we should make it a .py.in file to be able to put @PYTHON@ in the script?

> Even if installing python-xml (for the actual script) and python3
> (for the configure check) it complains about no input files or somesuch
> and fails....

Indeed, I sent a fix for that

https://gerrit.libreoffice.org/50071

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

On 21.02.2018 09:19, Samuel Thibault wrote:

> Rene Engelhard, on mer. 21 févr. 2018 09:11:02 +0100, wrote:
>>> diff --git a/bin/gla11y b/bin/gla11y
>>> new file mode 100755
>>> index 000000000000..d0619133ad0f
>>> --- /dev/null
>>> +++ b/bin/gla11y
>>> @@ -0,0 +1,216 @@
>>> +#!/usr/bin/env python
>>
>> That's "python". Python2.
>
> It works with either python2 or python3
>
>>> +    AC_MSG_CHECKING([for python lxml])
>>> +    if $PYTHON -c "import lxml.etree as ET" ; then
>>
>> Here it checks for lxml in the system python. This is a a 3.x.
>> Because for python3-uno in Debian, of course python3 is used.
>> And LOs internal python also is python3.
>>
>> But the actual script (see above) calls "python" and not "python3"
>
> Mmm, so we should make it a .py.in file to be able to put @PYTHON@ in the script?

Drop the shebang line and call it as `$(PYTHON) $(SRCDIR)/bin/gla11y`
from the makefile?
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

Stephan Bergmann, on mer. 21 févr. 2018 09:26:03 +0100, wrote:
> Drop the shebang line and call it as `$(PYTHON) $(SRCDIR)/bin/gla11y` from
> the makefile?

It's still be useful to keep the shebang for people to be also able to
run it directly as shell command, but we can force the python shell from
the makefile indeed, will submit that.

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Rene Engelhard Rene Engelhard
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

In reply to this post by Samuel Thibault-2
Hi,

On Wed, Feb 21, 2018 at 09:19:51AM +0100, Samuel Thibault wrote:
> > Even if installing python-xml (for the actual script) and python3
> > (for the configure check) it complains about no input files or somesuch
> > and fails....
>
> Indeed, I sent a fix for that
>
> https://gerrit.libreoffice.org/50071

OK, thanks - and then I wonder why this is ran in "normal" UIConfig targets and
not only in make check...

Regards,

Rene
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

Rene Engelhard, on mer. 21 févr. 2018 10:02:08 +0100, wrote:

> On Wed, Feb 21, 2018 at 09:19:51AM +0100, Samuel Thibault wrote:
> > > Even if installing python-xml (for the actual script) and python3
> > > (for the configure check) it complains about no input files or somesuch
> > > and fails....
> >
> > Indeed, I sent a fix for that
> >
> > https://gerrit.libreoffice.org/50071
>
> OK, thanks - and then I wonder why this is ran in "normal" UIConfig targets and
> not only in make check...

For the "error" cases (-W none), it makes sense:

« Add to the build process error checking (only the hard errors such as
bogus target names). »

That's really the kind of issues one should get as soon as compilation
time.

Later on I'll add a call in "make check" time for the warnings.

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Miklos Vajna-4 Miklos Vajna-4
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

Hi Samuel,

On Wed, Feb 21, 2018 at 10:11:18AM +0100, Samuel Thibault <[hidden email]> wrote:
> For the "error" cases (-W none), it makes sense:
>
> « Add to the build process error checking (only the hard errors such as
> bogus target names). »
>
> That's really the kind of issues one should get as soon as compilation
> time.
>
> Later on I'll add a call in "make check" time for the warnings.

For source code we have warnings for these linter type problems and then
--enable-werror (Jenkins uses it) turns those warnings into errors.

Probably the best would be if the .ui checker would follow the same
pattern, instead of running it twice during 'make check' (which is a
superset of 'make').

Thanks,

Miklos

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (188 bytes) Download Attachment
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

Hello,

Miklos Vajna, on jeu. 22 févr. 2018 09:53:43 +0100, wrote:

> On Wed, Feb 21, 2018 at 10:11:18AM +0100, Samuel Thibault <[hidden email]> wrote:
> > Rene Engelhard, on mer. 21 févr. 2018 10:02:08 +0100, wrote:
> > > OK, thanks - and then I wonder why this is ran in "normal" UIConfig targets and
> > > not only in make check...
> > For the "error" cases (-W none), it makes sense:
> >
> > « Add to the build process error checking (only the hard errors such as
> > bogus target names). »
> >
> > That's really the kind of issues one should get as soon as compilation
> > time.
> >
> > Later on I'll add a call in "make check" time for the warnings.
>
> For source code we have warnings for these linter type problems

Err, these are not just "linter type problems". They are undefined
references, just like undefined C references. So the failure belongs to
compilation time. If somebody modifies a .ui file in a way that gets
such undefined reference, compilation itself should really fail, just
like it does when a variable defintion is missing.

The "make check" warnings I mentioned above are *other* kinds of issues,
which would indeed only be tested within make check, made warnings by
default, and turned into errors by Jenkins.

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild

On 22.02.2018 13:54, Samuel Thibault wrote:

> Miklos Vajna, on jeu. 22 févr. 2018 09:53:43 +0100, wrote:
>> On Wed, Feb 21, 2018 at 10:11:18AM +0100, Samuel Thibault <[hidden email]> wrote:
>>> Rene Engelhard, on mer. 21 févr. 2018 10:02:08 +0100, wrote:
>>>> OK, thanks - and then I wonder why this is ran in "normal" UIConfig targets and
>>>> not only in make check...
>>> For the "error" cases (-W none), it makes sense:
>>>
>>> « Add to the build process error checking (only the hard errors such as
>>> bogus target names). »
>>>
>>> That's really the kind of issues one should get as soon as compilation
>>> time.
>>>
>>> Later on I'll add a call in "make check" time for the warnings.
>>
>> For source code we have warnings for these linter type problems
>
> Err, these are not just "linter type problems". They are undefined
> references, just like undefined C references. So the failure belongs to
> compilation time. If somebody modifies a .ui file in a way that gets
> such undefined reference, compilation itself should really fail, just
> like it does when a variable defintion is missing.
>
> The "make check" warnings I mentioned above are *other* kinds of issues,
> which would indeed only be tested within make check, made warnings by
> default, and turned into errors by Jenkins.

I don't understand the "turned into errors by Jenkins" part.  How would
that be done?

(And `make check` should ideally be "silent".  We do not want it to
produce any non-fatal warnings.)
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Invoking gla11y [was: core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild]

Stephan Bergmann, on jeu. 22 févr. 2018 16:43:37 +0100, wrote:

> On 22.02.2018 13:54, Samuel Thibault wrote:
> > Miklos Vajna, on jeu. 22 févr. 2018 09:53:43 +0100, wrote:
> > > On Wed, Feb 21, 2018 at 10:11:18AM +0100, Samuel Thibault <[hidden email]> wrote:
> > > > Rene Engelhard, on mer. 21 févr. 2018 10:02:08 +0100, wrote:
> > > > > OK, thanks - and then I wonder why this is ran in "normal" UIConfig targets and
> > > > > not only in make check...
> > > > For the "error" cases (-W none), it makes sense:
> > > >
> > > > « Add to the build process error checking (only the hard errors such as
> > > > bogus target names). »
> > > >
> > > > That's really the kind of issues one should get as soon as compilation
> > > > time.
> > > >
> > > > Later on I'll add a call in "make check" time for the warnings.
> > >
> > > For source code we have warnings for these linter type problems
> >
> > Err, these are not just "linter type problems". They are undefined
> > references, just like undefined C references. So the failure belongs to
> > compilation time. If somebody modifies a .ui file in a way that gets
> > such undefined reference, compilation itself should really fail, just
> > like it does when a variable defintion is missing.
> >
> > The "make check" warnings I mentioned above are *other* kinds of issues,
> > which would indeed only be tested within make check, made warnings by
> > default, and turned into errors by Jenkins.
>
> I don't understand the "turned into errors by Jenkins" part.  How would that
> be done?
>
> (And `make check` should ideally be "silent".  We do not want it to produce
> any non-fatal warnings.)

Well, perhaps I didn't understand what Miklos meant, then.

Miklos Vajna, on jeu. 22 févr. 2018 09:53:43 +0100, wrote:
> For source code we have warnings for these linter type problems and then
> --enable-werror (Jenkins uses it) turns those warnings into errors.
>
> Probably the best would be if the .ui checker would follow the same
> pattern, instead of running it twice during 'make check' (which is a
> superset of 'make').

What I had understood from that was that at "make" time there are only
compiler errors, and at "make check" time there are linter warnings, and
in Jenkins, --enable-werror is used to turn these warnings into errors.

Now that I re-read it with what you said, I understand that

- at "make" time there are some compiler warnings which are turned into
errors in Jenkins with --enable-werror
- Miklos suggested that we only call gla11y at "make" time, and not a
second time at "make check" time.

I'm fine with emitting accessibility warnings at "make" time and let
Jenkins turning them into errors, I just thought from the discussions at
FOSDEM that they should be done at "make check" time.

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: Invoking gla11y [was: core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild]

On 22.02.2018 17:08, Samuel Thibault wrote:

> Stephan Bergmann, on jeu. 22 févr. 2018 16:43:37 +0100, wrote:
>> On 22.02.2018 13:54, Samuel Thibault wrote:
>>> Miklos Vajna, on jeu. 22 févr. 2018 09:53:43 +0100, wrote:
>>>> On Wed, Feb 21, 2018 at 10:11:18AM +0100, Samuel Thibault <[hidden email]> wrote:
>>>>> Rene Engelhard, on mer. 21 févr. 2018 10:02:08 +0100, wrote:
>>>>>> OK, thanks - and then I wonder why this is ran in "normal" UIConfig targets and
>>>>>> not only in make check...
>>>>> For the "error" cases (-W none), it makes sense:
>>>>>
>>>>> « Add to the build process error checking (only the hard errors such as
>>>>> bogus target names). »
>>>>>
>>>>> That's really the kind of issues one should get as soon as compilation
>>>>> time.
>>>>>
>>>>> Later on I'll add a call in "make check" time for the warnings.
>>>>
>>>> For source code we have warnings for these linter type problems
>>>
>>> Err, these are not just "linter type problems". They are undefined
>>> references, just like undefined C references. So the failure belongs to
>>> compilation time. If somebody modifies a .ui file in a way that gets
>>> such undefined reference, compilation itself should really fail, just
>>> like it does when a variable defintion is missing.
>>>
>>> The "make check" warnings I mentioned above are *other* kinds of issues,
>>> which would indeed only be tested within make check, made warnings by
>>> default, and turned into errors by Jenkins.
>>
>> I don't understand the "turned into errors by Jenkins" part.  How would that
>> be done?
>>
>> (And `make check` should ideally be "silent".  We do not want it to produce
>> any non-fatal warnings.)
>
> Well, perhaps I didn't understand what Miklos meant, then.
>
> Miklos Vajna, on jeu. 22 févr. 2018 09:53:43 +0100, wrote:
>> For source code we have warnings for these linter type problems and then
>> --enable-werror (Jenkins uses it) turns those warnings into errors.
>>
>> Probably the best would be if the .ui checker would follow the same
>> pattern, instead of running it twice during 'make check' (which is a
>> superset of 'make').
>
> What I had understood from that was that at "make" time there are only
> compiler errors, and at "make check" time there are linter warnings, and
> in Jenkins, --enable-werror is used to turn these warnings into errors.
>
> Now that I re-read it with what you said, I understand that
>
> - at "make" time there are some compiler warnings which are turned into
> errors in Jenkins with --enable-werror
> - Miklos suggested that we only call gla11y at "make" time, and not a
> second time at "make check" time.
>
> I'm fine with emitting accessibility warnings at "make" time and let
> Jenkins turning them into errors, I just thought from the discussions at
> FOSDEM that they should be done at "make check" time.

`make` vs. `make check` is completely orthogonal to
--{en,dis}able-werror:  `make check` just runs more tests than `make`;
some tests are expensive (and some are a bit brittle), so there are
reasons why some builds want to be done without running the extra tests
(though in an ideal world, they would always be included).

--enable-werror causes C/C++ compiler warnings to be turned into build
failures.  We have a policy of a warning-free git master (and release
branches), so that developers can --enable-werror and immediately find
new issues they are introducing for which compilers emit warnings.  (And
to keep master warning free on all platforms, Gerrit/Jenkins builds are
also done with --enable-werror.)  Again, in an ideal world,
--enable-werror would always be enabled for every build, but there are
practical reasons (like some compiler emitting some hard-to-silence
warnings only in -O2 production builds) why some builds use
--disable-werror.

Now, gla11y shall do two things:

1  Find newly introduced bugs in .ui files.  Those should always cause
the build to fail, regardless of `make` vs. `make check` or
--enable-werror vs. --disable-werror.  I see no reason to have a build
mode in which those bugs are either not detected at all, or do not fail
the build.

2  Find existing bugs in .ui files (and maybe also minor newly
introduced issues that are not severe enough to be considered fatal bugs
upfront; I do not know).  Those should initially be suppressed by (1),
but should eventually all be fixed (IIUC).  So for somebody working on
fixing those issues, it would be useful to have a way to get a report of
all those issues.  Maybe a new make target would be useful for that.

Does that make sense?
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Re: Invoking gla11y [was: core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild]

Stephan Bergmann, on jeu. 22 févr. 2018 18:03:28 +0100, wrote:
> Does that make sense?

Yes, thanks for making these clear :)

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Samuel Thibault-2 Samuel Thibault-2
Reply | Threaded
Open this post in threaded view
|

Re: Invoking gla11y [was: core.git: bin/gla11y config_host.mk.in configure.ac solenv/gbuild]

In reply to this post by sberg
Stephan Bergmann, on jeu. 22 févr. 2018 18:03:28 +0100, wrote:
> Now, gla11y shall do two things:
>
> 1  Find newly introduced bugs in .ui files.
>
> 2  Find existing bugs in .ui files

For now, I have pushed this to gerrit
https://gerrit.libreoffice.org/50251

It introduces the suppresion mechanism and a suppression file for each
module. The build is silent except some

0 new error (2 suppressed by /mnt/sthibault/libreoffice/solenv/sanitizers/ui/svt.suppr)

I was indeed thinking that we could have a target which just runs gla11y
on all modules with ui files. I'm just not sure how it should plug into
makefiles, I guess just adding to gbuild_TARGETS but put the rules in
solenv/gbuild/UIConfig.mk?

Samuel
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice