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

Updated fix for Perl programs on Darwin #66446

Merged
merged 6 commits into from
Aug 19, 2019

Conversation

bdesham
Copy link
Contributor

@bdesham bdesham commented Aug 10, 2019

Motivation for this change

Fix biber (#35353) and exiftool (#63111) on Darwin.

This PR is just an updated version of PR #35477 by @boronine. That PR has merge conflicts now; this one fixes the conflicts. (@boronine: if you would rather pull these changes into your branch and rebase, please feel free to do so! I don’t want to seem as if I’m trying to take credit for your work.)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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.

--
Resolves: #35353 #35477 #47302 #62156 #63111

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Aug 10, 2019
@ofborg ofborg bot requested a review from ttuegel August 10, 2019 17:08
@bdesham
Copy link
Contributor Author

bdesham commented Aug 10, 2019

I don’t think it’s just a matter of patching, though. On my system the shebang line for the biber built from the latest Nixpkgs master is almost 50 KB (while the maximum macOS will tolerate is 512 bytes). I’m not sure how we’d get around this without introducing a wrapper. Can you be more specific about how we could just patch biber to fix it on macOS? patchShebangs doesn’t do anything to improve the length of the shebang line.

(The shebang line for exiftool contains a lot of redundant -I options, so it could be manipulated via sed to be less than 512 characters. But this approach is brittle and it wouldn’t work for biber anyway.)

@risicle
Copy link
Contributor

risicle commented Aug 11, 2019

Builds ok for me macos 10.13 FWIW

@gpampara
Copy link

Although the build works, the same error is present - the error is now just in the resulting .biber-wrapped script. The wrapper script does correctly set the environment though.

@bdesham
Copy link
Contributor Author

bdesham commented Aug 11, 2019

@volth I still don’t understand why creating an ad-hoc solution for each different program is preferable to creating a single wrapper that can be invoked when needed. Is there some concern with the closure size?

@bdesham
Copy link
Contributor Author

bdesham commented Aug 11, 2019

@gpampara Are you having that issue with the code from the PR, or one of the versions from the comments here?

The PR version of Biber works for me on macOS 10.13, at least to the extent that biber --help and biber --version work fine. The shebang lines for both scripts are nice and short:

$ head -n 1 result/bin/biber result/bin/.biber-wrapped 
==> result/bin/biber <==
#! /nix/store/5frlp3yii9vhxx6sc4daw3vzs7r3w7k5-bash-4.4-p23/bin/bash -e

==> result/bin/.biber-wrapped <==
#!/nix/store/3mxzksvycp6sk730zqqzch3qwla3hfhd-perl-5.28.2/bin/perl

@bdesham
Copy link
Contributor Author

bdesham commented Aug 11, 2019

@volth It seems like ack has the same problem on Darwin too—see #47302—and this comment implies that the youtube-viewer package is affected as well. There doesn’t seem to be a ticket for youtube-viewer but I went ahead and updated this PR to include a fix for ack too.

I’m not opposed to taking the ad-hoc approach if it really is only two packages that are affected, but if every Perl-based program has the potential to trigger this problem on Darwin then it seems like the fix should be part of the Nix Perl infrastructure, not something for each program to solve on its own. I understand that you don’t want to include a near-duplicate wrapper if it’s not necessary, but isn’t it going to be more total lines of code to fix the same problem in every single affected package?

@gpampara
Copy link

@bdesham the patch listings from the comments. I was hoping that they would provide a quick solution, but I'll apply your patches on a pinned version of nixpkgs locally :)

@bdesham
Copy link
Contributor Author

bdesham commented Aug 11, 2019

That seems reasonable to me. Just to be clear, you’re okay with adding a builder_darwin.sh script or something like that? Or should I try to put the logic (and OS checking) inline in the existing builder.sh?

@bdesham
Copy link
Contributor Author

bdesham commented Aug 11, 2019

I guess creating a new builder for Darwin would prevent a whole bunch of rebuilding on other platforms.

@bdesham
Copy link
Contributor Author

bdesham commented Aug 12, 2019

OK, I’ll work on a new Bash script we can call for these kinds of programs.

@bdesham bdesham force-pushed the updated-boronine-perl-darwin-fix branch from 9d2b8c9 to 162032b Compare August 18, 2019 05:00
@bdesham
Copy link
Contributor Author

bdesham commented Aug 18, 2019

I rewrote this PR to introduce a shell function called shortenPerlShebang. This function is invoked in the postInstall phase for ack, biber, exiftool, and youtube-viewer when building for Darwin. If this approach looks good then I will

  • mention this function in the manual,
  • add some inline documentation of the function,
  • add a check to ensure that the file being operated on actually does have a Perl shebang line, and
  • print a warning message if the resulting shebang line is still longer than 512 characters.

@ttuegel ttuegel removed their request for review August 18, 2019 17:38
@bdesham bdesham force-pushed the updated-boronine-perl-darwin-fix branch from 6deaed0 to 6f2bd80 Compare August 18, 2019 21:13
@bdesham
Copy link
Contributor Author

bdesham commented Aug 18, 2019

I’ve made the additional improvements I mentioned, so this should be good to go.

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

How was this tested? I get a following problem:

[nix-shell:~/.cache/nix-review/pr-66446]$ cd nixpkgs

[nix-shell:~/.cache/nix-review/pr-66446/nixpkgs]$ NIX_PATH=nixpkgs=`pwd` nix-shell -p biber --pure --run biber
/private/var/folders/1f/2mj_ng1n2y3_wc59fgw3fn_r0000gn/T/nix-shell-18812-0/rc: line 1: /nix/store/2rdg8x1vbrk839kngcbffchnm9847h7m-perl5.28.2-biber-2.12/bin/biber: Permission denied

[nix-shell:~/.cache/nix-review/pr-66446/nixpkgs]$ ls -la /nix/store/2rdg8x1vbrk839kngcbffchnm9847h7m-perl5.28.2-biber-2.12/bin/biber
-r--r--r-- 1 user wheel 41877 Dec 31  1969 /nix/store/2rdg8x1vbrk839kngcbffchnm9847h7m-perl5.28.2-biber-2.12/bin/biber

Seems like mv inherits permissions from the temp file.

@veprbl
Copy link
Member

veprbl commented Aug 18, 2019

This could be also useful for latexindent: #59537 (comment)

@bdesham bdesham force-pushed the updated-boronine-perl-darwin-fix branch from 43de9f7 to 347064b Compare August 18, 2019 22:26
@bdesham
Copy link
Contributor Author

bdesham commented Aug 18, 2019

@veprbl I did test the execution of the scripts at some point, but apparently I made other changes in the meantime that broke the files’ modes. Sorry about that! The issue should be fixed now.

@veprbl
Copy link
Member

veprbl commented Aug 18, 2019

Also should use nativeBuildInputs instead of buildInputs.

This setup hook modifies a Perl script so that any "-I" flags in its shebang
line are rewritten into a "use lib ..." statement on the next line. This gets
around a limitation in Darwin, which will not properly handle a script whose
shebang line exceeds 511 characters.
Also remove gnused from the buildInputs: it's already provided to us in
this stage.

Closes NixOS#47302.
@bdesham bdesham force-pushed the updated-boronine-perl-darwin-fix branch from 347064b to 8036b0f Compare August 18, 2019 23:23
@ofborg ofborg bot added the 6.topic: TeX Issues regarding texlive and TeX in general label Aug 19, 2019
@veprbl veprbl removed the 6.topic: TeX Issues regarding texlive and TeX in general label Aug 19, 2019
@veprbl veprbl merged commit 948025b into NixOS:master Aug 19, 2019
@veprbl
Copy link
Member

veprbl commented Aug 19, 2019

Thanks @bdesham @boronine @volth and others for tackling this bug!

@bdesham bdesham deleted the updated-boronine-perl-darwin-fix branch August 19, 2019 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildPerlModule broken on Darwin due to shebang character limit
4 participants