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

buildPerlPackage: fix patchShebangs when cross-compiling #95623

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

lopsided98
Copy link
Contributor

Motivation for this change

Cross-compiled perl.dev contains miniperl compiled for the build arch. When perl is added to buildInputs, perl.dev ends up in $HOST_PATH, causing patchShebangs to use ${perl.dev}/bin/perl as the shebang. A fix is to explicitly only include perl.out in buildInputs. This bug only occurs with Perl packages with executable scripts, for example XMLTwig.

This PR does not affect native builds, because native Perl does not have a dev output. I have only tested this PR with cross-compiled XMLTwig. I attempted to test git and hydra, but they both depend on ModuleBuild, which currently cannot be cross-compiled.

cc @alyssais @volth @worldofpeace

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 nixpkgs-review --run "nixpkgs-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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 16, 2020
@lopsided98
Copy link
Contributor Author

As far as I remember, It is intentional, many perl packets run perl as part of build process.

Yes, packages need miniperl at build time. perl.dev is added to nativeBuildInputs, which causes miniperl to be added to $PATH. $HOST_PATH, on the other hand, is only used by patchShebangs --host, and therefore must not have miniperl, which is what this PR fixes.

@lopsided98
Copy link
Contributor Author

@ofborg build pkgsCross.aarch64-multiplatform.perlPackages.XMLTwig pkgsCross.aarch64-multiplatform.perlPackages.XMLParser pkgsCross.aarch64-multiplatform.perlPackages.NetDBus pkgsCross.aarch64-multiplatform.perlPackages.FileSlurp

@lopsided98
Copy link
Contributor Author

lopsided98 commented Aug 17, 2020

Do you have a particular case?
A package which failed to cross-compile without this PR and succeeded with this PR?

XMLTwig is affected by this PR. It successfully cross-compiled, but the xml_grep script within the package had miniperl as the shebang, rather than perl built for the host. I found this while investigating unintentional build architecture references in a cross compiled NixOS system.

@lopsided98
Copy link
Contributor Author

Yeah, the or perl part is not necessary. I guess I was thinking that native Perl did not use multiple outputs.

The important part is that plain perl adds both perl.out and perl.dev to $HOST_PATH, which we can avoid by explicitly specifying perl.out.

@lopsided98
Copy link
Contributor Author

Yes, I think it would.

@lopsided98
Copy link
Contributor Author

I updated this PR to rename the dev output to mini. This does not cause any rebuilds for native Perl, and fixes the shebang in XMLTwig.

@ofborg build pkgsCross.armv7l-hf-multiplatform.perlPackages.XMLTwig pkgsCross.armv7l-hf-multiplatform.perlPackages.XMLParser pkgsCross.armv7l-hf-multiplatform.perlPackages.NetDBus pkgsCross.armv7l-hf-multiplatform.perlPackages.FileSlurp

@aanderse
Copy link
Member

@volth I'm sorry I don't do cross compilation so I don't know much about it. Then again if it has your approval it should probably be merged...

@shlevy
Copy link
Member

shlevy commented Aug 23, 2020

@Ericson2314 for general cross-compilation hygiene check

@@ -28,7 +28,7 @@ let

# TODO: Add a "dev" output containing the header files.
outputs = [ "out" "man" "devdoc" ] ++
optional crossCompiling "dev";
optional crossCompiling "mini";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly over paranoid, but might be nice to add a comment here that if we add a "mini" output to non-cross perl in the future we may want to check perl-modules/generic/default.nix to ensure it's still correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming mini to miniperl would make it more clear imho.

Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good to me, but don't have an env to properly test cross compilation.

@Ericson2314
Copy link
Member

This situation is very confusing me. There is a tradition of putting build-platform binaries in the "dev" output, but it is generally the last resort.

Here are my questions:

  1. Quick sanity check, does Perl have an target platform? It's an interpreter not compiler, so I don't think so.

  2. Is the "mini" output only a native perl?

  3. If it is just a build platform perl, why don't we use buildPackages.perl instead of perl.mini?

  4. If it does also have some host platform things, we can use propgatedNativeBuildInputs to indicate those host platform things will need to be used with some other build platform things without polluting the HOST_PATH.

@stigtsp
Copy link
Member

stigtsp commented Aug 23, 2020

I've built XML::Twig on aarch64-linux using pkgCross and QEMU and getting different results:

Building using QEMU gives a working xml_grep executable.

$ nix-build -A perlPackages.XMLTwig --argstr system aarch64-linux
...
$ head -1 result/bin/xml_grep
#!/nix/store/bpakd2xsfxv5gdpq4pshjir8l5di7knw-perl-5.30.3/bin/perl -w -I/nix/store/bpakd2xsfxv5gdpq4pshjir8l5di7knw-perl-5.30.3/lib/perl5/site_perl -I/nix/store/gqy8jqg0bilai2lhz1asr3dhdgmx0vfa-perl5.30.3-XML-Parser-2.44/lib/perl5/site_perl -I/nix/store/mrdhwviprylq0bxkn6x04mh01fkmq3h6-perl5.30.3-libwww-perl-6.45/lib/perl5/site_perl -I/nix/store/di9mpjrwx8wxbqqmkv24x1f14n51ypfd-perl5.30.3-File-Listing-6.04/lib/perl5/site_perl -I/nix/store/382d8gjy6cx9b8f8nmllk6fwn7ykwzfn-perl5.30.3-HTTP-Date-6.05/lib/perl5/site_perl [...]

However on pkgCross, dependencies for xml_grep are missing from the shebang.

$ nix-build -A pkgsCross.aarch64-multiplatform.perlPackages.XMLTwig
..
$ head -1 result/bin/xml_grep
#!/nix/store/cpxrf7znnw2vx04bgp1l334z5x913dn3-perl-5.30.3-aarch64-unknown-linux-gnu/bin/perl -w -I/nix/store/bpg8nxy56lnbymr77b9bch08w1794p73-perl5.30.3-XML-Twig-3.52-aarch64-unknown-linux-gnu/lib/perl5/site_perl

@lopsided98
Copy link
Contributor Author

The Perl cross infrastructure in nixpkgs is based on the perl-cross project which provides a custom build system and patches to make it possible to cross-compile Perl.

@Ericson2314

1. Quick sanity check, does Perl have an target platform? It's an interpreter not compiler, so I don't think so.

Perl has native (XS) modules, although I know very little about the build process.

2. Is the "mini" output only a native perl?

It also contains pure Perl stubs for native modules that are needed at build time.

3. If it is just a build platform perl, why don't we use `buildPackages.perl` instead of `perl.mini`?

miniperl is wrapped to add certain directories from the host perl to PERL5LIB. It might be possible to use buildPackages.perl instead of miniperl, but at the very least the search paths would have to overridden to match miniperl's.

4. If it does also have some host platform things, we can use `propgatedNativeBuildInputs` to indicate those host platform things will need to be used with some other build platform things without polluting the `HOST_PATH`.

I'm not sure what this would entail in this case.

@stigtsp

However on pkgCross, dependencies for xml_grep are missing from the shebang.

This is because the cross setup hook uses $targetOffset instead of $hostOffset. I don't know why this was done nor why this PR broke the existing behavior, but changing it to $hostOffset fixes the shebang, and doesn't obviously break any of the packages we have been testing.

@stigtsp
Copy link
Member

stigtsp commented Aug 24, 2020

However on pkgCross, dependencies for xml_grep are missing from the shebang.

This is because the cross setup hook uses $targetOffset instead of $hostOffset. I don't know why this was done nor why this PR broke the existing behavior, but changing it to $hostOffset fixes the shebang [..]

Tested that pkgsCross.aarch64-multiplatform.perlPackages.XMLTwig builds xml_grep with correct shebang, and that it executes OK.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, well, this looks good. Reading https://arsv.github.io/perl-cross/design.html it looks like mini Perl is a holdover from the regular build. It shouldn't be needed at all for the cross build as it is just for bootstrapping, but I am fully sympathetic that Perl was too much of a mess to fix that.

Because it is weird, it's good that we renamed the output so we can figure out what's using it.

@Ericson2314 Ericson2314 merged commit 6cbd695 into NixOS:master Aug 24, 2020
@lopsided98 lopsided98 deleted the perl-cross-shebang branch August 24, 2020 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants