Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git: fix the "perlSupport = false" configuration #74213

Merged
merged 1 commit into from Nov 28, 2019

Conversation

@ruuda
Copy link
Contributor

ruuda commented Nov 25, 2019

When perlSupport = false, we will set NO_PERL=1, and build Git without Perl support. This is a build option that Git supports. However, Git's test suite still requires a Perl to be available to run the tests, and we did not provide one. The tests respect PERL_PATH, and if it is not set, they default to /usr/bin/perl.

Before this commit, if we set perlSupport = false, then no Perl would be available to the package, and so the tests would default to /usr/bin/perl. When building without a sandbox, that could still work,
even though there is no perl on the path, because the tests defaulted to an absolute path.

You can reproduce this issue as follows:

nix-build -E 'let pkgs = (import ./default.nix) {}; in pkgs.git.override { perlSupport = false; }'

I just ran into this when trying to build pkgs.git from an old version of Nixpkgs that I was able to build just fine in the past, and today it would not build any more, complaining when running the tests:

make -C t/ all
make[1]: Entering directory '/build/git-2.18.0/t'
rm -f -r 'test-results'
/nix/store/czx8vkrb9jdgjyz8qfksh10vrnqa723l-bash-4.4-p23/bin/bash: /usr/bin/perl: No such file or directory

In the past the sandbox was not enabled by default, so then it worked for me. But now that it is enabled, my host's (not NixOS) /usr/bin/perl is no longer accessible, and the build fails.

The solution is to explicitly set PERL_PATH when running the tests. This almost works, except that there appears to be a bug in the test for git request-pull. That command is a Bash script that calls Perl at some point, so it requires Perl, and therefore it cannot be supported when NO_PERL=1. But that particular test does not check whether Git was compiled with Perl support (other tests do include that check), and that makes the test fail:

t5150-request-pull.sh ..............................
not ok 4 - pull request after push
not ok 5 - request asks HEAD to be pulled
not ok 6 - pull request format
not ok 7 - request-pull ignores OPTIONS_KEEPDASHDASH poison
not ok 9 - pull request with mismatched object
not ok 10 - pull request with stale object
Dubious, test returned 1 (wstat 256, 0x100)
Failed 6/10 subtests

This output makes sense if you look at t5150-request-pull.sh. Test 1 and 2 are setup steps. Test 3 does call request-pull, but it expects the command to fail, and it cannot distinguish between the command exiting with a nonzero exit code, or failing to start it at all. So test 3 passes for the wrong reasons. Test 4 through 10 all call request-pull, so they fail.

The quick workaround here is to disable the test. I will look into upstreaming a patch that makes the test skip itself when Perl is disabled.

Motivation for this change

Without sandbox, setting perlSupport = false creates an implicit dependency on the host system. With sandbox, building with perlSupport = false fails.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I also verified that this does not introduce a Perl dependency in the closure with nix-store -qR.

Notify maintainers

cc @peti @the-kenny @wmertens @globin

Also, @bgamari, as the author of perlSupport, you might be interested in this.

Copy link
Contributor

wmertens left a comment

LGTM - but I propose you change the target branch to staging.

When perlSupport = false, we will set NO_PERL=1, and build Git without
Perl support. This is a build option that Git supports. However, Git's
test suite still requires a Perl to be available to run the tests, and
we did not provide one. The tests respect PERL_PATH, and if it is not
set, they default to /usr/bin/perl.

Before this commit, if we set "perlSupport = false", then no Perl would
be available to the package, and so the tests would default to
/usr/bin/perl. When building without a sandbox, that could still work,
even though there is no "perl" on the path, because the tests defaulted
to an absolute path.

You can reproduce this issue as follows:

    nix-build -E 'let pkgs = (import ./default.nix) {}; in pkgs.git.override { perlSupport = false; }'

I just ran into this when trying to build pkgs.git from an old version
of Nixpkgs that I was able to build just fine in the past, and today it
would not build any more, complaining when running the tests:

    make -C t/ all
    make[1]: Entering directory '/build/git-2.18.0/t'
    rm -f -r 'test-results'
    /nix/store/czx8vkrb9jdgjyz8qfksh10vrnqa723l-bash-4.4-p23/bin/bash: /usr/bin/perl: No such file or directory

In the past the sandbox was not enabled by default, so then it worked
for me. But now that it is enabled, my host's (not NixOS) /usr/bin/perl
is no longer accessible, and the build fails.

The solution is to explicitly set PERL_PATH when running the tests. This
*almost* works, except that there appears to be a bug in the test for
"git request-pull". That command is a Bash script that calls Perl at
some point, so it requires Perl, and therefore it cannot be supported
when NO_PERL=1. But that particular test does not check whether Git was
compiled with Perl support (other tests do include that check), and that
makes the test fail:

    t5150-request-pull.sh ..............................
    not ok 4 - pull request after push
    not ok 5 - request asks HEAD to be pulled
    not ok 6 - pull request format
    not ok 7 - request-pull ignores OPTIONS_KEEPDASHDASH poison
    not ok 9 - pull request with mismatched object
    not ok 10 - pull request with stale object
    Dubious, test returned 1 (wstat 256, 0x100)
    Failed 6/10 subtests

This output makes sense if you look at t5150-request-pull.sh. Test 1 and
2 are setup steps. Test 3 does call request-pull, but it expects the
command to fail, and it cannot distinguish between the command exiting
with a nonzero exit code, or failing to start it at all. So test 3
passes for the wrong reasons. Test 4 through 10 all call request-pull,
so they fail.

The quick workaround here is to disable the test. I will look into
upstreaming a patch that makes the test skip itself when Perl is
disabled.
@ruuda ruuda changed the base branch from master to staging Nov 27, 2019
@ruuda ruuda force-pushed the ruuda:fix-git-perl-support branch from 895108c to 2163fc7 Nov 27, 2019
@ruuda
Copy link
Contributor Author

ruuda commented Nov 27, 2019

Thanks for having a look! I rebased this on top of staging and changed the base branch.

@ofborg ofborg bot requested a review from wmertens Nov 27, 2019
@wmertens wmertens merged commit acfa5d8 into NixOS:staging Nov 28, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
git on aarch64-linux Success
Details
git on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@wmertens
Copy link
Contributor

wmertens commented Nov 28, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.