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

Fix pkgs.biber on Darwin #35477

Closed
wants to merge 1 commit into from
Closed

Fix pkgs.biber on Darwin #35477

wants to merge 1 commit into from

Conversation

boronine
Copy link
Contributor

@boronine boronine commented Feb 24, 2018

Fix for #35353

  • As part of this PR, the fix is applied ONLY to pkgs.biber package
  • The fix is can be enabled for other packages (ack, perlcritic, youtube-viewer etc.) using the flag doUseWrapper in the perl-modules builder
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@Amar1729
Copy link
Contributor

Tested this a little on Darwin. Out of the current perlPackages in all-packages:
Working pkgs on current master:

  • exiftool
  • ack
  • perlcritic
  • youtube-viewer

Working using this PR:

  • exiftool
  • ack
  • perlcritic
  • youtube-viewer

Just wanted to bump this because of #47302

@kirelagin
Copy link
Member

I am having this issue with biber. The fix seems reasonable, any idea why it is marked WIP?
Would be great to get this merged.

@veprbl
Copy link
Member

veprbl commented Mar 20, 2019

This needs to be rebased to staging

@boronine
Copy link
Contributor Author

I rebased this PR but only enabled the fix for biber for now. Enabling it for all packages would certainly result in some breakage, as suggested by @volth here:
#58158 (comment)

@boronine
Copy link
Contributor Author

I tested the change on MacOS and Ubuntu. I think it should be merge-able now.

@veprbl
Copy link
Member

veprbl commented Apr 11, 2019

Making a copy of a builder for just a single package is not an optimal solution from the maintenance perspective. Ideally, the workaround would just override the phases or pre/post hooks of the biber derivation. If you really need to do something with the builder, you need to modify that common builder and not make a new one with a small modification.

@boronine
Copy link
Contributor Author

@veprbl @volth I have 2 solutions for this issue in 2 separate PRs.

(FYI: original problem is described here: #35353)

Solution 1: #35477 (this PR)

  • Create new builder.sh file that avoids shebang issue
  • New builder disabled by default, enabled only for pkgs.biber
  • Pro: only rebuild pkgs.biber
  • Con: code duplication in two builder files

Solution 2: #62156

  • Modify existing builder.sh to accept doUseWrapper flag
  • Wrapper-based build disabled by default, enabled only for pkgs.biber
  • Pro: avoid code duplication
  • Con: need to rebuild everything, I'm not sure how to test this or verify apart from code review

I believe @veprbl prefers solution 2. I don't mind either way. I'm just keeping solution 1 around for now since it's more practical for people needing biber and similar broken Perl packages today.

If either of these solutions gets merged, there remains an open question of whether the shebang hack needs to be removed completely and if so, how to do it safely. Due to limited time I cannot commit to this larger project. However, even if it remains unsolved, merging one of these solutions will allow simple fixes for ack, perlcritic, youtube-viewer and other Perl packages on Darwin.

@boronine boronine mentioned this pull request May 28, 2019
10 tasks
@veprbl
Copy link
Member

veprbl commented May 28, 2019

@boronine
First of all, thank you for working on this.

Con: need to rebuild everything, I'm not sure how to test this or verify apart from code review

Single time rebuild is not an issue, especially if it is limited to perl related stuff. This can be tested by building a few packages and comparing the build outputs to the original. This will be also built on Hydra and if there is a major breakage that will be visible. So there is no downside here actually.

Con: code duplication in two builder files

Even if solution 1 would be merged, I'm pretty sure someone soon would pick up the work on deduplication of those builders to get it into the shape of solution 2 for the reason described above.

@boronine
Copy link
Contributor Author

@veprbl thank you for the explanation and your help with this!

I'll keep this PR open to make sure @Amar1729's findings don't get lost. Once the initial solution makes it to master, I'll make a new PR with additional package fixes.

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.

None yet

6 participants