Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built

Ramkumar Ramachandra [off-list ref] writes:

What this means is that git-remote-testpy is not built by default (when
'make' is invoked), but t5800 runs by default (like every other test in
t/).  As a result, a new contributor cloning git.git and running 'make
test' for the first time will notice test failures.

Sounds like making "make test" build it is a more correct approach,
at least to me.  What am I missing?

Junio C Hamano wrote:

Sounds like making "make test" build it is a more correct approach,
at least to me.  What am I missing?

How exactly?  I'm not exactly competent in make, but this is what I
understood from what you said (and it's obviously wrong):

diff --git a/Makefile b/Makefile
index 03524d0..da91937 100644
--- a/Makefile
+++ b/Makefile

@@ -580,6 +580,7 @@ TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-wildmatch

 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS += git-remote-testpy

 # List built-in command $C whose implementation cmd_$C() is not in
 # builtin/$C.o but is linked in as part of some other command.

On Wed, Jun 19, 2013 at 02:05:06AM +0530, Ramkumar Ramachandra wrote:

I'm confused. git-remote-testpy is already mentioned in SCRIPT_PYTHON,
which means it should be built by "make" or "make all", as well as "make
test" (which depends on "all"). I just double checked that this is the
case with a fresh clone of master. NO_INSTALL should not have an impact.

But upon looking at the Makefile more, I am doubly confused. We build
$(ALL_PROGRAMS), which contains $(SCRIPTS), which contains
$(SCRIPT_PYTHON_INS), the set of _installed_ python scripts. Which
doesn't make sense; we would want to build all of the generated scripts,
and only care about the installed ones for the "install" target.

Ah, I see. We later add back in $(NO_INSTALL) as dependencies of "all".
That is perhaps not the most direct way of doing it, but I suspect it
was done to keep the meaning of "$(ALL_PROGRAMS)" the same before and
after.

So I do not see any problem with the current Makefile. Running "make" or
"make test" should let t5800 pass. Can you describe how you are
triggering the issue in more detail?

-Peff

Jeff King wrote:

So I do not see any problem with the current Makefile. Running "make" or
"make test" should let t5800 pass. Can you describe how you are
triggering the issue in more detail?

master, at the time of reporting the issue:

  artagnon|master=:~/src/git$ make -j 8
  artagnon|master=:~/src/git$ rm git-remote-testpy
  artagnon|master=:~/src/git$ make -j 8
  artagnon|master=:~/src/git$ cd t
  artagnon|master=:~/src/git$ sh t5800-remote-testpy.sh
  # failure

Yesterday's jc publish fixed it: 6c473a5 (build: generate and clean
test scripts, 2013-06-07) graduated to master; it adds $NO_INSTALL to
the target "all", among other things.

[...]

Yes, I agree that it is convoluted and can be cleaned up.

On Fri, Jun 21, 2013 at 12:07:50PM +0530, Ramkumar Ramachandra wrote:

Yesterday's jc publish fixed it: 6c473a5 (build: generate and clean
test scripts, 2013-06-07) graduated to master; it adds $NO_INSTALL to
the target "all", among other things.

Ah, makes sense. Sorry to be slow.

-Peff