{"thread":{"id":"34207","subject":"[PATCH] t/t5800-remote-testpy: skip all tests if not built","startedAt":"2013-06-18T20:06:46Z","lastAt":"2013-06-21T06:40:49Z","messageCount":6,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221297","messageId":"1371586006-16289-1-git-send-email-artagnon@gmail.com","threadId":"34207","inReplyTo":null,"subject":"[PATCH] t/t5800-remote-testpy: skip all tests if not built","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-18T20:06:46Z","receivedAt":"2013-06-18T20:06:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"416fda6 (build: do not install git-remote-testpy, 2013-05-24) added\ngit-remote-testpy to NO_INSTALL, with the intent of excluding it from\nthe default install.  This is typically meant for scripts in contrib/\nwith their own testsuite and Makefile (e.g. git remote-bzr, git\nremote-hg).  Unfortunately, git-remote-testpy.py is a toplevel script\nwithout its own Makefile; its testsuite is located in\nt/t5800-remote-testpy.sh.\n\nWhat this means is that git-remote-testpy is not built by default (when\n'make' is invoked), but t5800 runs by default (like every other test in\nt/).  As a result, a new contributor cloning git.git and running 'make\ntest' for the first time will notice test failures.  Fix this\nunpleasantness by patching t5800 to check for git-remote-testpy as a\nprerequisite.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n Candidate for maint?\n\n t/t5800-remote-testpy.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5800-remote-testpy.sh b/t/t5800-remote-testpy.sh\nindex 1e683d4..9df35b5 100755\n--- a/t/t5800-remote-testpy.sh\n+++ b/t/t5800-remote-testpy.sh\n@@ -12,6 +12,11 @@ if ! test_have_prereq PYTHON ; then\n \ttest_done\n fi\n \n+if ! test -e \"$GIT_BUILD_DIR/git-remote-testpy\"; then\n+\tskip_all='skipping python remote-helper tests, git-remote-testpy not built'\n+\ttest_done\n+fi\n+\n \"$PYTHON_PATH\" -c '\n import sys\n if sys.hexversion < 0x02040000:\n-- \n1.8.3.1.379.g1f0e7d7\n"},{"id":"221298","messageId":"7vip1b40zb.fsf@alter.siamese.dyndns.org","threadId":"34207","inReplyTo":"1371586006-16289-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-18T20:28:56Z","receivedAt":"2013-06-18T20:28:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> What this means is that git-remote-testpy is not built by default (when\n> 'make' is invoked), but t5800 runs by default (like every other test in\n> t/).  As a result, a new contributor cloning git.git and running 'make\n> test' for the first time will notice test failures.\n\nSounds like making \"make test\" build it is a more correct approach,\nat least to me.  What am I missing?\n"},{"id":"221299","messageId":"CALkWK0=6ot9ZXEyMmO1ZtEXPi5H1JXXvG5j6yvBeE56H9zQBwg@mail.gmail.com","threadId":"34207","inReplyTo":"7vip1b40zb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-18T20:35:06Z","receivedAt":"2013-06-18T20:35:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Sounds like making \"make test\" build it is a more correct approach,\n> at least to me.  What am I missing?\n\nHow exactly?  I'm not exactly competent in make, but this is what I\nunderstood from what you said (and it's obviously wrong):\n\ndiff --git a/Makefile b/Makefile\nindex 03524d0..da91937 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -580,6 +580,7 @@ TEST_PROGRAMS_NEED_X += test-svn-fe\n TEST_PROGRAMS_NEED_X += test-wildmatch\n\n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n+TEST_PROGRAMS += git-remote-testpy\n\n # List built-in command $C whose implementation cmd_$C() is not in\n # builtin/$C.o but is linked in as part of some other command.\n"},{"id":"221555","messageId":"20130621053112.GA9634@sigill.intra.peff.net","threadId":"34207","inReplyTo":"CALkWK0=6ot9ZXEyMmO1ZtEXPi5H1JXXvG5j6yvBeE56H9zQBwg@mail.gmail.com","subject":"Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-21T05:31:12Z","receivedAt":"2013-06-21T05:31:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2013 at 02:05:06AM +0530, Ramkumar Ramachandra wrote:\n\n> Junio C Hamano wrote:\n> > Sounds like making \"make test\" build it is a more correct approach,\n> > at least to me.  What am I missing?\n> \n> How exactly?  I'm not exactly competent in make, but this is what I\n> understood from what you said (and it's obviously wrong):\n> \n> diff --git a/Makefile b/Makefile\n> index 03524d0..da91937 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -580,6 +580,7 @@ TEST_PROGRAMS_NEED_X += test-svn-fe\n>  TEST_PROGRAMS_NEED_X += test-wildmatch\n> \n>  TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n> +TEST_PROGRAMS += git-remote-testpy\n\nI'm confused. git-remote-testpy is already mentioned in SCRIPT_PYTHON,\nwhich means it should be built by \"make\" or \"make all\", as well as \"make\ntest\" (which depends on \"all\"). I just double checked that this is the\ncase with a fresh clone of master. NO_INSTALL should not have an impact.\n\nBut upon looking at the Makefile more, I am doubly confused. We build\n$(ALL_PROGRAMS), which contains $(SCRIPTS), which contains\n$(SCRIPT_PYTHON_INS), the set of _installed_ python scripts. Which\ndoesn't make sense; we would want to build all of the generated scripts,\nand only care about the installed ones for the \"install\" target.\n\nAh, I see. We later add back in $(NO_INSTALL) as dependencies of \"all\".\nThat is perhaps not the most direct way of doing it, but I suspect it\nwas done to keep the meaning of \"$(ALL_PROGRAMS)\" the same before and\nafter.\n\nSo I do not see any problem with the current Makefile. Running \"make\" or\n\"make test\" should let t5800 pass. Can you describe how you are\ntriggering the issue in more detail?\n\n-Peff\n"},{"id":"221558","messageId":"CALkWK0ky+zYk+7s8yK6VeA+2xyQ41TfoREym2jq3L77q5MTpzA@mail.gmail.com","threadId":"34207","inReplyTo":"20130621053112.GA9634@sigill.intra.peff.net","subject":"Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-21T06:37:50Z","receivedAt":"2013-06-21T06:37:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> So I do not see any problem with the current Makefile. Running \"make\" or\n> \"make test\" should let t5800 pass. Can you describe how you are\n> triggering the issue in more detail?\n\nmaster, at the time of reporting the issue:\n\n  artagnon|master=:~/src/git$ make -j 8\n  artagnon|master=:~/src/git$ rm git-remote-testpy\n  artagnon|master=:~/src/git$ make -j 8\n  artagnon|master=:~/src/git$ cd t\n  artagnon|master=:~/src/git$ sh t5800-remote-testpy.sh\n  # failure\n\nYesterday's jc publish fixed it: 6c473a5 (build: generate and clean\ntest scripts, 2013-06-07) graduated to master; it adds $NO_INSTALL to\nthe target \"all\", among other things.\n\n> [...]\n\nYes, I agree that it is convoluted and can be cleaned up.\n"},{"id":"221559","messageId":"20130621064049.GA29922@sigill.intra.peff.net","threadId":"34207","inReplyTo":"CALkWK0ky+zYk+7s8yK6VeA+2xyQ41TfoREym2jq3L77q5MTpzA@mail.gmail.com","subject":"Re: [PATCH] t/t5800-remote-testpy: skip all tests if not built","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-21T06:40:49Z","receivedAt":"2013-06-21T06:40:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 21, 2013 at 12:07:50PM +0530, Ramkumar Ramachandra wrote:\n\n> Yesterday's jc publish fixed it: 6c473a5 (build: generate and clean\n> test scripts, 2013-06-07) graduated to master; it adds $NO_INSTALL to\n> the target \"all\", among other things.\n\nAh, makes sense. Sorry to be slow.\n\n-Peff\n"}]}