| Message ID | 20161220211603.28510-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 20 December 2016 at 13:16, Ben Pfaff <blp@ovn.org> wrote: > A previous patch fixed double rebuilds when running tests in some cases. > That patch removed dependencies from targets in tests/automake.mk that > were redundant because the "all" target already depended on them. A > dependency on tests/atlocal was also removed in the belief that "all" > depended on tests/atlocal. This belief was incorrect, which meant that > tests/atlocal would not get regenerated if it was removed or out of date. > This commit fixes the problem. > > Reported-by: Joe Stringer <joe@ovn.org> > Fixes: a8cb456227b0 ("tests: Fix double-rebuild of testsuite for "check-valgrind" and similar.") > Signed-off-by: Ben Pfaff <blp@ovn.org> Thanks for the fix! Seems to work fine, I'll let you know if I see any further trouble with it. Does tests/atconfig need the same attention? Acked-by: Joe Stringer <joe@ovn.org>
On Tue, Dec 20, 2016 at 03:18:43PM -0800, Joe Stringer wrote: > On 20 December 2016 at 13:16, Ben Pfaff <blp@ovn.org> wrote: > > A previous patch fixed double rebuilds when running tests in some cases. > > That patch removed dependencies from targets in tests/automake.mk that > > were redundant because the "all" target already depended on them. A > > dependency on tests/atlocal was also removed in the belief that "all" > > depended on tests/atlocal. This belief was incorrect, which meant that > > tests/atlocal would not get regenerated if it was removed or out of date. > > This commit fixes the problem. > > > > Reported-by: Joe Stringer <joe@ovn.org> > > Fixes: a8cb456227b0 ("tests: Fix double-rebuild of testsuite for "check-valgrind" and similar.") > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Thanks for the fix! Seems to work fine, I'll let you know if I see any > further trouble with it. > > Does tests/atconfig need the same attention? > > Acked-by: Joe Stringer <joe@ovn.org> Thanks for the review. I applied this to master, adding your ack. It's not clear to me whether tests/atconfig needs the same attention. It depends on how tests/atlocal was causing a problem. What caused it to need to be regenerated? Some possible reasons don't apply to atconfig; for example, what appears in atconfig depends only on decisions made during "configure", which is also what creates atconfig, so there's never a reason to regenerate atconfig unless something randomly deletes it, which shouldn't normally happen.
On 20 December 2016 at 16:28, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Dec 20, 2016 at 03:18:43PM -0800, Joe Stringer wrote: >> On 20 December 2016 at 13:16, Ben Pfaff <blp@ovn.org> wrote: >> > A previous patch fixed double rebuilds when running tests in some cases. >> > That patch removed dependencies from targets in tests/automake.mk that >> > were redundant because the "all" target already depended on them. A >> > dependency on tests/atlocal was also removed in the belief that "all" >> > depended on tests/atlocal. This belief was incorrect, which meant that >> > tests/atlocal would not get regenerated if it was removed or out of date. >> > This commit fixes the problem. >> > >> > Reported-by: Joe Stringer <joe@ovn.org> >> > Fixes: a8cb456227b0 ("tests: Fix double-rebuild of testsuite for "check-valgrind" and similar.") >> > Signed-off-by: Ben Pfaff <blp@ovn.org> >> >> Thanks for the fix! Seems to work fine, I'll let you know if I see any >> further trouble with it. >> >> Does tests/atconfig need the same attention? >> >> Acked-by: Joe Stringer <joe@ovn.org> > > Thanks for the review. I applied this to master, adding your ack. > > It's not clear to me whether tests/atconfig needs the same attention. > It depends on how tests/atlocal was causing a problem. What caused it > to need to be regenerated? Some possible reasons don't apply to > atconfig; for example, what appears in atconfig depends only on > decisions made during "configure", which is also what creates atconfig, > so there's never a reason to regenerate atconfig unless something > randomly deletes it, which shouldn't normally happen. For atlocal, I was modifying tests/atlocal.in to add a new variable, and in the tests/system-traffic.at tests the variables were not being made available. I didn't know about atconfig, I just saw that the referred patch also removed the target for that one. Looking at the contents, it doesn't seem like it is likely to change much.
On Tue, Dec 20, 2016 at 04:32:05PM -0800, Joe Stringer wrote: > On 20 December 2016 at 16:28, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Dec 20, 2016 at 03:18:43PM -0800, Joe Stringer wrote: > >> On 20 December 2016 at 13:16, Ben Pfaff <blp@ovn.org> wrote: > >> > A previous patch fixed double rebuilds when running tests in some cases. > >> > That patch removed dependencies from targets in tests/automake.mk that > >> > were redundant because the "all" target already depended on them. A > >> > dependency on tests/atlocal was also removed in the belief that "all" > >> > depended on tests/atlocal. This belief was incorrect, which meant that > >> > tests/atlocal would not get regenerated if it was removed or out of date. > >> > This commit fixes the problem. > >> > > >> > Reported-by: Joe Stringer <joe@ovn.org> > >> > Fixes: a8cb456227b0 ("tests: Fix double-rebuild of testsuite for "check-valgrind" and similar.") > >> > Signed-off-by: Ben Pfaff <blp@ovn.org> > >> > >> Thanks for the fix! Seems to work fine, I'll let you know if I see any > >> further trouble with it. > >> > >> Does tests/atconfig need the same attention? > >> > >> Acked-by: Joe Stringer <joe@ovn.org> > > > > Thanks for the review. I applied this to master, adding your ack. > > > > It's not clear to me whether tests/atconfig needs the same attention. > > It depends on how tests/atlocal was causing a problem. What caused it > > to need to be regenerated? Some possible reasons don't apply to > > atconfig; for example, what appears in atconfig depends only on > > decisions made during "configure", which is also what creates atconfig, > > so there's never a reason to regenerate atconfig unless something > > randomly deletes it, which shouldn't normally happen. > > For atlocal, I was modifying tests/atlocal.in to add a new variable, > and in the tests/system-traffic.at tests the variables were not being > made available. > > I didn't know about atconfig, I just saw that the referred patch also > removed the target for that one. Looking at the contents, it doesn't > seem like it is likely to change much. OK, in that case there's no reason to add atconfig, because there's no .in file that can change. Thanks, Ben.
diff --git a/tests/automake.mk b/tests/automake.mk index 4939d7c..f4d4879 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -112,6 +112,8 @@ SYSTEM_TESTSUITE_AT = \ tests/system-ovn.at \ tests/system-traffic.at +check_SCRIPTS += tests/atlocal + TESTSUITE = $(srcdir)/tests/testsuite TESTSUITE_PATCH = $(srcdir)/tests/testsuite.patch SYSTEM_KMOD_TESTSUITE = $(srcdir)/tests/system-kmod-testsuite
A previous patch fixed double rebuilds when running tests in some cases. That patch removed dependencies from targets in tests/automake.mk that were redundant because the "all" target already depended on them. A dependency on tests/atlocal was also removed in the belief that "all" depended on tests/atlocal. This belief was incorrect, which meant that tests/atlocal would not get regenerated if it was removed or out of date. This commit fixes the problem. Reported-by: Joe Stringer <joe@ovn.org> Fixes: a8cb456227b0 ("tests: Fix double-rebuild of testsuite for "check-valgrind" and similar.") Signed-off-by: Ben Pfaff <blp@ovn.org> --- tests/automake.mk | 2 ++ 1 file changed, 2 insertions(+)