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@xxxxxxxxxx> > > > > 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@xxxxxxxxx> > > > > > > --- > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel