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

netpbm: 10.89.1 → 10.91.2 #97045

Merged
merged 5 commits into from Oct 5, 2020
Merged

netpbm: 10.89.1 → 10.91.2 #97045

merged 5 commits into from Oct 5, 2020

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 3, 2020

Motivation for this change

Tried updating if it makes cross compilation better.

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.

@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 3, 2020

While reading the build system sources, I noticed it also uses vendored Jasper. We might want to use the Fedora patch to unbundle it or better find a way to disable it since Jasper is full of security holes.

@kampka
Copy link
Contributor

kampka commented Sep 7, 2020

Cross-building works for the most part, but I found some iregularitues with shebangs, eg.

$ nix-build . -A pkgsCross.aarch64-multiplatform.netpbm
$ head result-bin/bin/ppmquant                                                               
#! /nix/store/j206pskg2yzyla1cnfrb9kb5n5bfvjgm-bash-4.4-p23/bin/bash -e

Otherwise, nicely done 👍

@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 7, 2020 via email

@kampka
Copy link
Contributor

kampka commented Sep 7, 2020

Could you clarify what the irregularities are?

Of course. After cross building from x86 to aarch64, the shebang should point to an aarch64 version of bash, otherwise the script would probably run into an exec error.
The bash it points to however is the one of the build system

$ file /nix/store/j206pskg2yzyla1cnfrb9kb5n5bfvjgm-bash-4.4-p23/bin/bash
/nix/store/j206pskg2yzyla1cnfrb9kb5n5bfvjgm-bash-4.4-p23/bin/bash: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/2pi6zgkwnr3zdslvlv16nixpzvbyjx1n-glibc-2.31/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, not stripped

@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 7, 2020 via email

@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 7, 2020 via email

@kampka
Copy link
Contributor

kampka commented Sep 7, 2020

Weird. I would expect the patchShebangsAuto hook to do the right thing wrt cross compilation. OR is the wrapProgram causing the issue?

It looks like patchShebang does the wrong thing here as the bash reference is already a reference to the Nix store.
This would probably be worth fixing at that level.

@jtojnar
Copy link
Contributor Author

jtojnar commented Sep 7, 2020 via email

@kampka
Copy link
Contributor

kampka commented Sep 7, 2020

A practical solution (not necessarily pretty) would be to just patch it over like so.

jtojnar and others added 5 commits October 2, 2020 13:05
Also fix accidentally not running postConfigure
Especially since Jasper is unmaintained and insecure
Co-Authored-By: Christian Kampka <christian@kampka.net>
@jtojnar
Copy link
Contributor Author

jtojnar commented Oct 2, 2020

@GrahamcOfBorg build netpbm pkgsCross.aarch64-multiplatform.netpbm

@jtojnar
Copy link
Contributor Author

jtojnar commented Oct 2, 2020

@Ericson2314 @matthewbauer any idea how to make makeWrapper use the correct shell for shebang?

@jtojnar jtojnar merged commit b0f40be into NixOS:master Oct 5, 2020
@jtojnar jtojnar deleted the netpbm-10.91.2 branch October 5, 2020 20:25
@Ericson2314
Copy link
Member

@jtojnar yeah I recently learned that makeWrapper is set up wrong. it should use targetPackages.runtimeShellso we usedepsBuildBuildvsnativeBuildInputsto control what shell is used. Instead people have been putting it inbuildInputs` to achieve the same result which isn't right when its only a build-time dep.

I should make an issue for this.

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

3 participants