[Spice-devel] [PATCH server v2 03/13] tests: make sure all tests are built on default rule

[Jonathon Jongsma]

On Fri, 2016-01-15 at 09:08 -0500, Frediano Ziglio wrote:
> > 
> > Hi
> > 
> > ----- Original Message -----
> > > > 
> > > > Hi
> > > > 
> > > > On Fri, Jan 15, 2016 at 11:50 AM, Frediano Ziglio <fziglio at redhat.com>
> > > > wrote:
> > > > > 
> > > > > > Subject: [PATCH server v2 03/13] tests: make sure all tests are
> > > > > > built
> > > > > > on
> > > > > > default rule
> > > > > > 
> > > > > > We should have all or none, I don't see the point of having just
> > > > > > some
> > > > > > of
> > > > > > them.
> > > > > > 
> > > > > 
> > > > > I don't know why but this description (and subject) looks odd to me.
> > > > > 
> > > > > I would put something like (if I understood it):
> > > > > 
> > > > > "Programs listed in noinst_PROGRAMS inherit default rule/settings to
> > > > > list
> > > > > all
> > > > > tests program also in this macro"
> > > > > 
> > > > 
> > > > I don't mind changing the text, but I really don't understand yours ;)
> > > > 
> > > 
> > > That means I didn't get the reason of the change.
> > > 
> > > What's a rule for you?
> > > 
> > > output: source
> > >       do_something_to_generate_output_from_source
> > > 
> > > is a rule
> > > 
> > > TESTS = xxx
> > > 
> > > is a variable (also called macro)
> > 
> > Yes
> > 
> > the default rule being "all", it builds noinst_PROGRAMS but not
> > check_PROGRAMS. I prefer either build all with noinst_PROGRAMS or none (in
> > which case we should remove some of them duplicated in check_PROGRAMS)
> > 
> 
> Really didn't get it before :)
> Can you post again with an updated comment?
> 
> Frediano


I didn't fully understand the original comment either (nor did I really
understand Frediano's suggested alternative ;). 

I think that even a simple re-wording would make it a little more clear:

"Make sure that the default rule builds all tests."


> 
> > > 
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > > > ---
> > > > > >  server/tests/Makefile.am | 15 +++++++--------
> > > > > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> > > > > > index 15196f9..007930c 100644
> > > > > > --- a/server/tests/Makefile.am
> > > > > > +++ b/server/tests/Makefile.am
> > > > > > @@ -35,6 +35,12 @@ LDADD =
> > > > > > \
> > > > > >       $(SPICE_NONPKGCONFIG_LIBS)                              \
> > > > > >       $(NULL)
> > > > > > 
> > > > > > +TESTS =                                              \
> > > > > > +     stat_test                               \
> > > > > > +     stream-test                             \
> > > > > > +     test-qxl-parsing                        \
> > > > > > +     $(NULL)
> > > > > > +
> > > > > 
> > > > > Why did you moved above? This just make the patch bigger.
> > > > > 
> > > > 
> > > > It's saner/simpler to define before use.
> > > > 
> > > 
> > > Let's say that is more similar to assignment and other languages so
> > > it's easier to read by people not very friendly with Makefiles
> > > (and honestly I think that many developers does not really like
> > > Makefiles).
> > > 
> > > > > >  noinst_PROGRAMS =                            \
> > > > > >       test_display_no_ssl                     \
> > > > > >       test_display_streaming                  \
> > > > > > @@ -47,14 +53,7 @@ noinst_PROGRAMS =                          \
> > > > > >       test_vdagent                            \
> > > > > >       test_display_width_stride               \
> > > > > >       spice-server-replay                     \
> > > > > > -     stream-test                             \
> > > > > > -     stat_test                               \
> > > > > > -     $(NULL)
> > > > > > -
> > > > > > -TESTS =                                              \
> > > > > > -     stat_test                               \
> > > > > > -     test-qxl-parsing                        \
> > > > > > -     stream-test                             \
> > > > > > +     $(TESTS)                                \
> > > > > >       $(NULL)
> > > > > > 
> > > > > >  check_PROGRAMS = $(TESTS)
> > > > > > --
> > > > > > 2.5.0
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Frediano
> > > > 
> > > > 
> > > > 
> > > > --
> > > > Marc-André Lureau
> > > > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel