5:09 a.m.
On Thu, Sep 19, 2019 at 09:36:35AM +0100, Daniel P. Berrangé wrote:
On Thu, Sep 19, 2019 at 10:20:04AM +0200, Pavel Hrdina wrote:
> On Thu, Sep 19, 2019 at 12:05:23AM +0200, Fabiano Fidêncio wrote:
> > On Wed, Sep 18, 2019 at 9:57 PM Ján Tomko <jtomko(a)redhat.com> wrote:
> > >
> > > On Wed, Sep 18, 2019 at 05:40:06PM +0200, Fabiano Fidêncio wrote:
> > > >On Wed, Sep 18, 2019 at 1:22 AM Fabiano Fidêncio
<fidencio(a)redhat.com> wrote:
> > > >>
> > > >> On Tue, Sep 17, 2019 at 8:17 PM Pavel Hrdina
<phrdina(a)redhat.com> wrote:
> > > >> >
> > > >> > On Tue, Sep 17, 2019 at 06:53:30PM +0200, Fabiano Fidêncio
wrote:
> > > >> > > On Tue, Sep 17, 2019 at 5:22 PM Pavel Hrdina
<phrdina(a)redhat.com> wrote:
> > > >> > This is an intentional change to run syntax-check tests for
dist target
> > > >> > as well, but it might be possible to configure it somehow.
Anyway, I
> > > >> > would rather prefer to run syntax-check when running ninja
dist than
> > > >> > having it as a separate target.
> > > >>
> > > >> osinfo-db-tools has it, libosinfo has it, libvirt-jenkins-ci uses
it.
> > > >> This is a pattern already being used and here I have a strong
> > > >> preference for not changing the pattern if we can easily adapt to
it.
> > > >
> > > >Just for the record, we had a face-to-face discussion about this and
> > >
> > > Can you record the reasoning as well?
> >
> > A few topics were brought up:
> > - It's closer to what different projects using meson are doing
> > (systemd was the example used);
Not a fan of using systemd as an example O:-)
> > - It runs on `ninja dist` without having to run an
additional command
> > (`ninja syntax-check`);
I think this is the compelling reason that I didn't understand before.
"ninja dist" is *NOT* the same as "make dist" from autoconf world.
It is in fact equivalent to 'make distcheck'. If we have syntx-check
as a separate target, then we're loosing a safety check at time of
release.
I thought that was exactly the point of keeping the target separate -
not to run the tests meant for patch submissions. For a downstream
example, I remember us not caring that much for exact spacing or failing
copyright year check on our RHEL branches.
(For example QEMU has these checks separate and only ran on the new
patch, not the whole codebase)
> > - If the test is named 'syntax-check', the test can still be run as
> > `meson test syntax-check`;
> >
> > Everything else is just about personal preference. And talking about
> > personal preference, I'd rather keep the syntax-check target, it looks
> > cleaner to me ... however, my personal preference is far from counting
> > something on "libvirt land" and I'm okay on adapting osinfo
related
> > projects to whatever is decided here.
> >
> > There are projects, though, which do not have any tests (gtk-vnc, for
> > example) and those will have to add a single test case to run
> > syntax-check in order to adapt to this decision. Up to the maintainers
> > to decide what to do ... but consistence, IMHO, is important.
>
> A bit more to the reasoning behind dropping syntax-check target.
>
> I was looking at a lot of meson-based projects [1] and none of the
> projects that I've checked have syntax-check target and they are using
> only the test target.
>
> I was trying to search any usage of syntax-check but it looks like it's
> a gnulib specific target, only few projects have this target defined in
> Makefile.
>
> Another argument from my side was that we require syntax-check and check
> to be executed before posting patches to mailing list, having it under
> single target would simplify things for new contributors and would align
> more with meson community and projects.
>
I like this, hopefully we'll get less patches failing syntax-check from
the old contributors as well.
Jano
> Dan pointed out an issue with our CI that we would loose the
separation
> in our CI results, which can be solved by using 'suite' labels for tests
> that we run, so for syntax-check we can use 'syntax' label and for unit
> tests we can use 'unit' label, there can be multiple labels assigned to
> each test and to run only a set of tests with a specific label we just
> need to run these commands:
>
> meson test --suite syntax
> meson test --suite unit
>
> This way we would still have the separation in our CI and contributors
> could easily run `meson test` or `ninja test` to run everything.
Yes, that's a usable approach.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|