# [PATCH] t/t5800-remote-testpy: skip all tests if not built 6 messages from 2013-06-18 to 2013-06-21. Participants: Ramkumar Ramachandra, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/34207 ## Ramkumar Ramachandra, 2013-06-18 20:06 Subject: [PATCH] t/t5800-remote-testpy: skip all tests if not built Message-ID: <1371586006-16289-1-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371586006-16289-1-git-send-email-artagnon%40gmail.com ``` 416fda6 (build: do not install git-remote-testpy, 2013-05-24) added git-remote-testpy to NO_INSTALL, with the intent of excluding it from the default install. This is typically meant for scripts in contrib/ with their own testsuite and Makefile (e.g. git remote-bzr, git remote-hg). Unfortunately, git-remote-testpy.py is a toplevel script without its own Makefile; its testsuite is located in t/t5800-remote-testpy.sh. 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. Fix this unpleasantness by patching t5800 to check for git-remote-testpy as a prerequisite. Signed-off-by: Ramkumar Ramachandra --- Candidate for maint? t/t5800-remote-testpy.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t5800-remote-testpy.sh b/t/t5800-remote-testpy.sh index 1e683d4..9df35b5 100755 --- a/t/t5800-remote-testpy.sh +++ b/t/t5800-remote-testpy.sh @@ -12,6 +12,11 @@ if ! test_have_prereq PYTHON ; then test_done fi +if ! test -e "$GIT_BUILD_DIR/git-remote-testpy"; then + skip_all='skipping python remote-helper tests, git-remote-testpy not built' + test_done +fi + "$PYTHON_PATH" -c ' import sys if sys.hexversion < 0x02040000: -- 1.8.3.1.379.g1f0e7d7 ``` ## Junio C Hamano, 2013-06-18 20:28 Subject: Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built Message-ID: <7vip1b40zb.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vip1b40zb.fsf%40alter.siamese.dyndns.org In-Reply-To: <1371586006-16289-1-git-send-email-artagnon@gmail.com> ``` Ramkumar Ramachandra 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? ``` ## Ramkumar Ramachandra, 2013-06-18 20:35 Subject: Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built Message-ID: URL: https://gitlist.dev/e/CALkWK0%3D6ot9ZXEyMmO1ZtEXPi5H1JXXvG5j6yvBeE56H9zQBwg%40mail.gmail.com In-Reply-To: <7vip1b40zb.fsf@alter.siamese.dyndns.org> ``` 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. ``` ## Jeff King, 2013-06-21 05:31 Subject: Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built Message-ID: <20130621053112.GA9634@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130621053112.GA9634%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Jun 19, 2013 at 02:05:06AM +0530, Ramkumar Ramachandra wrote: > 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 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 ``` ## Ramkumar Ramachandra, 2013-06-21 06:37 Subject: Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built Message-ID: URL: https://gitlist.dev/e/CALkWK0ky%2BzYk%2B7s8yK6VeA%2B2xyQ41TfoREym2jq3L77q5MTpzA%40mail.gmail.com In-Reply-To: <20130621053112.GA9634@sigill.intra.peff.net> ``` 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. ``` ## Jeff King, 2013-06-21 06:40 Subject: Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built Message-ID: <20130621064049.GA29922@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130621064049.GA29922%40sigill.intra.peff.net In-Reply-To: ``` 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 ```