{"thread":{"id":"52336","subject":"[PATCH] t5150: skip request-pull test if Perl is disabled","startedAt":"2019-11-26T00:08:41Z","lastAt":"2019-12-16T18:32:33Z","messageCount":11,"participants":["Ruud van Asseldonk","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"387024","messageId":"4f11b5b3-a68e-642a-c5fb-7b5dae698669@veniogames.com","threadId":"52336","inReplyTo":null,"subject":"[PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Ruud van Asseldonk","fromEmail":"dev@veniogames.com","sentAt":"2019-11-26T00:02:46Z","receivedAt":"2019-11-26T00:08:41Z","isPatch":true,"sender":{"key":"dev@veniogames.com","avatar":"https://avatars.githubusercontent.com/u/506953?v=4"},"body":"The git-request-pull.sh script invokes Perl, so it requires Perl to be\navailable, but the associated test t5150 does not skip itself when Perl\nhas been disabled, which then makes subtest 4 through 10 fail. Subtest 3\nstill passes, but for the wrong reasons (it expects git-request-pull to\nfail, and it does fail when Perl is not available). The initial two\nsubtests that do pass are only doing setup.\n\nTo prevent t5150 from failing the build when NO_PERL=1, add a check that\nsets skip_all when \"! test_have_prereq PERL\", just like how for example\nt3701-add-interactive skips itself when Perl has been disabled.\n\nSigned-off-by: Ruud van Asseldonk \n---\nI discovered this issue in the Git package in Nixpkgs. The Nix package\nmanager tries to make it hard to accidentally introduce undeclared\ndependencies, and it has a sandbox that hides things in /usr/bin. So\nwhen it builds with NO_PERL=1, it really makes no Perl available, so you\ncannot accidentally depend on the Perl in /usr/bin/perl. For the tests\nit does set PERL_PATH, but it does not point to a binary in /usr/bin.\n\n t/t5150-request-pull.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh\nindex 852dcd913f..1ad4ecc29a 100755\n--- a/t/t5150-request-pull.sh\n+++ b/t/t5150-request-pull.sh\n@@ -4,6 +4,12 @@ test_description='Test workflows involving pull request.'\n \n . ./test-lib.sh\n \n+if ! test_have_prereq PERL\n+then\n+\tskip_all='skipping request-pull tests, perl not available'\n+\ttest_done\n+fi\n+\n test_expect_success 'setup' '\n \n \tgit init --bare upstream.git &&\n-- \n2.24.0\n"},{"id":"387028","messageId":"20191126004314.GC14157@google.com","threadId":"52336","inReplyTo":"4f11b5b3-a68e-642a-c5fb-7b5dae698669@veniogames.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-26T00:43:14Z","receivedAt":"2019-11-26T00:43:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRuud van Asseldonk wrote:\n\n> The git-request-pull.sh script invokes Perl,\n\nDo you have more details? From a quick glance, I wasn't able to find\nany instance of perl or $PERL_PATH in git-request-pull.sh.\n\nThanks,\nJonathan\n"},{"id":"387030","messageId":"20191126004647.GD14157@google.com","threadId":"52336","inReplyTo":"4f11b5b3-a68e-642a-c5fb-7b5dae698669@veniogames.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-26T00:46:47Z","receivedAt":"2019-11-26T00:46:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ruud van Asseldonk wrote:\n\n> The git-request-pull.sh script invokes Perl,\n\nOkay, on second glance I found it:\n\n set fnord $(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"${remote:-HEAD}\" \"$headrev\")\n\nThis does seem pretty inherently to require perl, so makes sense.\n\nI wonder if we can generalize this. For example, would it make sense to\nhave a helper that looks for @@PERL@@ in a file, so we could say\n\n\tif uses_perl git-request-pull.sh && ! test_have_prereq PERL\n\tthen\n\t\t...\n\tfi\n\nThat way, this would be more futureproof in case someone eliminates\nthe perl dependency (either by improving that particular parsing step\nor by rewriting the whole program in C).\n\nIn any event,\nReviewed-by: Jonathan Nieder \n\nDo the request-pull tests in t7006-pager.sh need the same treatment?\n\nThanks,\nJonathan\n"},{"id":"387139","messageId":"20191127112150.GA22221@sigill.intra.peff.net","threadId":"52336","inReplyTo":"4f11b5b3-a68e-642a-c5fb-7b5dae698669@veniogames.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-27T11:21:50Z","receivedAt":"2019-11-27T11:21:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 26, 2019 at 01:02:46AM +0100, Ruud van Asseldonk wrote:\n\n> The git-request-pull.sh script invokes Perl, so it requires Perl to be\n> available, but the associated test t5150 does not skip itself when Perl\n> has been disabled, which then makes subtest 4 through 10 fail. Subtest 3\n> still passes, but for the wrong reasons (it expects git-request-pull to\n> fail, and it does fail when Perl is not available). The initial two\n> subtests that do pass are only doing setup.\n> \n> To prevent t5150 from failing the build when NO_PERL=1, add a check that\n> sets skip_all when \"! test_have_prereq PERL\", just like how for example\n> t3701-add-interactive skips itself when Perl has been disabled.\n> \n> Signed-off-by: Ruud van Asseldonk \n> ---\n> I discovered this issue in the Git package in Nixpkgs. The Nix package\n> manager tries to make it hard to accidentally introduce undeclared\n> dependencies, and it has a sandbox that hides things in /usr/bin. So\n> when it builds with NO_PERL=1, it really makes no Perl available, so you\n> cannot accidentally depend on the Perl in /usr/bin/perl. For the tests\n> it does set PERL_PATH, but it does not point to a binary in /usr/bin.\n\nHmm. I don't think that technique gives complete coverage. There are\nother scripts (e.g., filter-branch) that call a bare \"perl\" (not\nPERL_PATH), which presumably pass the tests even though they'd break in\na real-world system without perl. In fact, many scripts used to do this\nbefore fcb06a8d54 (use @@PERL@@ in built scripts, 2013-10-28). I don't\nthink the effects on NO_PERL were really considered there; it was more\nabout finding the right perl.\n\nI think NO_PERL has historically mostly meant \"do not build or install\nperl scripts\", and not \"everything ought to run fine without perl\".\nWe've generally assumed you can run vanilla perl snippets from the\ncommand line the same way you'd run awk or sed (and the tests use this\nextensively, which is why you have to set PERL_PATH again to run them).\n\nThat said, most of those casual uses of perl in actual built scripts\nhave gone away because the shell scripts have gone away. It looks like\nfilter-branch, request-pull, and instaweb are the last holdouts. So\nmaybe we should be treating NO_PERL as disabling those scripts, too.\n\nBut then, should we be doing more to make it clear that those scripts\nare broken in a NO_PERL build? Who knows what happens if you run\nfilter-branch without any perl available?\n\n-Peff\n"},{"id":"387251","messageId":"a5fb708d-e7b6-b149-7b21-07649487fb65@veniogames.com","threadId":"52336","inReplyTo":"20191126004647.GD14157@google.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Ruud van Asseldonk","fromEmail":"dev@veniogames.com","sentAt":"2019-11-27T22:42:21Z","receivedAt":"2019-11-27T22:42:32Z","isPatch":true,"sender":{"key":"dev@veniogames.com","avatar":"https://avatars.githubusercontent.com/u/506953?v=4"},"body":"Thanks for having a look Jonathan.\n\nJonathan Nieder wrote:\n> Ruud van Asseldonk wrote:\n> \n>> The git-request-pull.sh script invokes Perl,\n> \n> Okay, on second glance I found it:\n> \n> set fnord $(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"${remote:-HEAD}\" \"$headrev\")\n> \n> This does seem pretty inherently to require perl, so makes sense.\n> \n> I wonder if we can generalize this. For example, would it make sense to\n> have a helper that looks for @@PERL@@ in a file, so we could say\n> \n> \tif uses_perl git-request-pull.sh && ! test_have_prereq PERL\n> \tthen\n> \t\t...\n> \tfi\n> \n> That way, this would be more futureproof in case someone eliminates\n> the perl dependency (either by improving that particular parsing step\n> or by rewriting the whole program in C).\n\nThat sounds like a good idea. How about turning that into a test itself? \nThe test would check that the script mentions @@PERL@@, as the first \nsubtest. That way, if somebody removes the Perl dependency in the \nfuture, that will make the test fail, and that will be a reminder to \nremove the check from the test, so the test runs unconditionally.\n\n> In any event,\n> Reviewed-by: Jonathan Nieder \n> \n> Do the request-pull tests in t7006-pager.sh need the same treatment?\n\nI think these tests are fine, they run \"git -p request-pull\", not enough \narguments to make request-pull do something, so it prints its usage and \nexits with 1. The tests use test_must_fail to verify the exit code, and \nprinting usage does go through the pager. request-pull does not call \nPerl when it only prints its usage.\n"},{"id":"387253","messageId":"55d0bcd1-1f15-46ff-42f1-be87eedc2e11@veniogames.com","threadId":"52336","inReplyTo":"20191127112150.GA22221@sigill.intra.peff.net","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Ruud van Asseldonk","fromEmail":"dev@veniogames.com","sentAt":"2019-11-27T23:45:40Z","receivedAt":"2019-11-27T23:45:49Z","isPatch":true,"sender":{"key":"dev@veniogames.com","avatar":"https://avatars.githubusercontent.com/u/506953?v=4"},"body":"> Hmm. I don't think that technique gives complete coverage. There are\n> other scripts (e.g., filter-branch) that call a bare \"perl\" (not\n> PERL_PATH), which presumably pass the tests even though they'd break in\n> a real-world system without perl. In fact, many scripts used to do this\n> before fcb06a8d54 (use @@PERL@@ in built scripts, 2013-10-28). I don't\n> think the effects on NO_PERL were really considered there; it was more\n> about finding the right perl.\n\nHmm, filter-branch calls bare \"perl\", not @@PERL@@ or Perl at an \nabsolute path. At first I thought that made it pass when \"perl\" is on \nthe PATH in tests. But even when I replace those with @@PERL@@, the test \nstill pass. I will investigate further.\n\n> I think NO_PERL has historically mostly meant \"do not build or install\n> perl scripts\", and not \"everything ought to run fine without perl\".\n> We've generally assumed you can run vanilla perl snippets from the\n> command line the same way you'd run awk or sed (and the tests use this\n> extensively, which is why you have to set PERL_PATH again to run them)\n> That said, most of those casual uses of perl in actual built scripts\n> have gone away because the shell scripts have gone away. It looks like\n> filter-branch, request-pull, and instaweb are the last holdouts. So\n> maybe we should be treating NO_PERL as disabling those scripts, too.\n> \n> But then, should we be doing more to make it clear that those scripts\n> are broken in a NO_PERL build? Who knows what happens if you run\n> filter-branch without any perl available?\n\nMy understanding was that NO_PERL controlled the runtime dependencies of \nGit, and that the tests require Perl either way. Of course, without Perl \nany scripts that depend on it can't be used, but like you say, there are \nfew of them left. I think it would make sense to not install those \nscripts when NO_PERL=1. Should I make a patch to change that in the \nmakefile?\n\nBest,\nRuud\n"},{"id":"387262","messageId":"20191128013111.GA76989@google.com","threadId":"52336","inReplyTo":"20191127112150.GA22221@sigill.intra.peff.net","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-28T01:31:11Z","receivedAt":"2019-11-28T01:31:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Hmm. I don't think that technique gives complete coverage. There are\n> other scripts (e.g., filter-branch) that call a bare \"perl\" (not\n> PERL_PATH), which presumably pass the tests even though they'd break in\n> a real-world system without perl. In fact, many scripts used to do this\n> before fcb06a8d54 (use @@PERL@@ in built scripts, 2013-10-28). I don't\n> think the effects on NO_PERL were really considered there; it was more\n> about finding the right perl.\n\nOh! Thanks for looking deeper.\n\n> I think NO_PERL has historically mostly meant \"do not build or install\n> perl scripts\", and not \"everything ought to run fine without perl\".\n> We've generally assumed you can run vanilla perl snippets from the\n> command line the same way you'd run awk or sed (and the tests use this\n> extensively, which is why you have to set PERL_PATH again to run them).\n\nRight. PERL_PATH and NO_PERL are more orthogonal than I had thought.\nSo this is\n\n Not-Reviewed-by: Jonathan Nieder \n\n--- the patch shouldn't be applied as is.\n\n> That said, most of those casual uses of perl in actual built scripts\n> have gone away because the shell scripts have gone away. It looks like\n> filter-branch, request-pull, and instaweb are the last holdouts. So\n> maybe we should be treating NO_PERL as disabling those scripts, too.\n>\n> But then, should we be doing more to make it clear that those scripts\n> are broken in a NO_PERL build? Who knows what happens if you run\n> filter-branch without any perl available?\n\nAgreed: if we want to follow this approach, we should install stubs in\nplace of those scripts when NO_PERL=YesPlease. Will say more about\nthis in a separate reply.\n\nThanks for catching it,\nJonathan\n"},{"id":"387263","messageId":"20191128013501.GB76989@google.com","threadId":"52336","inReplyTo":"55d0bcd1-1f15-46ff-42f1-be87eedc2e11@veniogames.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-28T01:35:01Z","receivedAt":"2019-11-28T01:35:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRuud van Asseldonk wrote:\n> Jeff King wrote:\n\n>> I think NO_PERL has historically mostly meant \"do not build or install\n>> perl scripts\", and not \"everything ought to run fine without perl\".\n>> We've generally assumed you can run vanilla perl snippets from the\n>> command line the same way you'd run awk or sed (and the tests use this\n>> extensively, which is why you have to set PERL_PATH again to run them)\n\nWe've definitely held the stance that NO_PERL doesn't mean to disable\nperl in tests. For casual use of perl in installed shell scripts, on\nthe other hand, I suspect it was more that we forgot about them than\nthat we had decided one way or another. :)\n\n[...]\n>> That said, most of those casual uses of perl in actual built scripts\n>> have gone away because the shell scripts have gone away. It looks like\n>> filter-branch, request-pull, and instaweb are the last holdouts. So\n>> maybe we should be treating NO_PERL as disabling those scripts, too.\n>>\n>> But then, should we be doing more to make it clear that those scripts\n>> are broken in a NO_PERL build? Who knows what happens if you run\n>> filter-branch without any perl available?\n>\n> My understanding was that NO_PERL controlled the runtime dependencies of\n> Git, and that the tests require Perl either way. Of course, without Perl any\n> scripts that depend on it can't be used, but like you say, there are few of\n> them left. I think it would make sense to not install those scripts when\n> NO_PERL=1. Should I make a patch to change that in the makefile?\n\nYes, that sounds good to me. (The status quo also seems fine to me,\nbut I like that this proposed approach would simplify things a\nlittle.)\n\nThanks for working on this, and sorry for the earlier noise.\n\nSincerely,\nJonathan\n"},{"id":"387386","messageId":"xmqqblsrz1uk.fsf@gitster-ct.c.googlers.com","threadId":"52336","inReplyTo":"20191128013111.GA76989@google.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-02T06:19:15Z","receivedAt":"2019-12-02T06:19:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Not-Reviewed-by: Jonathan Nieder \n>\n> --- the patch shouldn't be applied as is.\n> ...\n> Agreed: if we want to follow this approach, we should install stubs in\n> place of those scripts when NO_PERL=YesPlease. Will say more about\n> this in a separate reply.\n\nI am just leaving a note here in the thread to make sure I notice if\nthere is any progress/conclusion, until which time I'll keep the\npatch on hold. Thanks.\n"},{"id":"388081","messageId":"20191213074659.GA95694@coredump.intra.peff.net","threadId":"52336","inReplyTo":"xmqqblsrz1uk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-13T07:46:59Z","receivedAt":"2019-12-13T07:47:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 01, 2019 at 10:19:15PM -0800, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> > Not-Reviewed-by: Jonathan Nieder \n> >\n> > --- the patch shouldn't be applied as is.\n> > ...\n> > Agreed: if we want to follow this approach, we should install stubs in\n> > place of those scripts when NO_PERL=YesPlease. Will say more about\n> > this in a separate reply.\n> \n> I am just leaving a note here in the thread to make sure I notice if\n> there is any progress/conclusion, until which time I'll keep the\n> patch on hold. Thanks.\n\nThinking on this more, it might not be a bad idea to take Ruud's initial\npatch here. It certainly makes things better for his NO_PERL case now,\nand then in the future we can either:\n\n - stop building request-pull entirely with NO_PERL, but we'd still need\n the tests to realize that we shouldn't be testing it\n\n - change request-pull to not require perl, at which point we'd remove\n this restriction\n\n-Peff\n"},{"id":"388287","messageId":"xmqqsglkjf45.fsf@gitster-ct.c.googlers.com","threadId":"52336","inReplyTo":"20191213074659.GA95694@coredump.intra.peff.net","subject":"Re: [PATCH] t5150: skip request-pull test if Perl is disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-16T18:32:26Z","receivedAt":"2019-12-16T18:32:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Dec 01, 2019 at 10:19:15PM -0800, Junio C Hamano wrote:\n>\n>> Jonathan Nieder writes:\n>> \n>> > Not-Reviewed-by: Jonathan Nieder \n>> >\n>> > --- the patch shouldn't be applied as is.\n>> > ...\n>> > Agreed: if we want to follow this approach, we should install stubs in\n>> > place of those scripts when NO_PERL=YesPlease. Will say more about\n>> > this in a separate reply.\n>> \n>> I am just leaving a note here in the thread to make sure I notice if\n>> there is any progress/conclusion, until which time I'll keep the\n>> patch on hold. Thanks.\n>\n> Thinking on this more, it might not be a bad idea to take Ruud's initial\n> patch here. It certainly makes things better for his NO_PERL case now,\n> and then in the future we can either:\n>\n> - stop building request-pull entirely with NO_PERL, but we'd still need\n> the tests to realize that we shouldn't be testing it\n>\n> - change request-pull to not require perl, at which point we'd remove\n> this restriction\n\nHmph, that is a reasonable stance to take, I would think. Let's\nmove it forward.\n\nThanks.\n"}]}