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

libargon2: fixed cross-compilation #67490

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@kisik21
Copy link
Contributor

commented Aug 26, 2019

Motivation for this change

Makefile had a hardcoded unprefixed ar. I wrote a patch and added an optional make flag to override it in case we're cross-compiling.

Unfortunately, this causes a rebuild of native packages.

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.
Notify maintainers

cc @Radvendii @olynch

@aanderse

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Looks like this patch should be an upstream fix. Can you open a ticket upstream please?

kisik21 added a commit to kisik21/phc-winner-argon2 that referenced this pull request Aug 26, 2019
Some Linux distributions (e.g. NixOS, where this issue was spotted) don't provide an unprefixed ar when cross-compiling. This PR aims to fix this.

See[NixOS/nixpkgs#67490](NixOS/nixpkgs#67490) for information on where did it start.
@kisik21

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Copy link
Contributor

left a comment

That is great you could submit the fix upstream! Thanks!

pkgs/development/libraries/libargon2/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libargon2/default.nix Outdated Show resolved Hide resolved
@kisik21 kisik21 force-pushed the kisik21:master branch 2 times, most recently from fdc705f to 53b1281 Aug 27, 2019
@kisik21

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

Accidentally forgot to commit stuff when rewriting my commit. Sorry! :3

@kisik21 kisik21 force-pushed the kisik21:master branch 2 times, most recently from 96a20f1 to 61f54e1 Aug 27, 2019
@kisik21

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

Ugh... I'm so clumsy when it comes to rewriting my pull requests. I'm terribly sorry for overloading OfBorg

Copy link
Contributor

left a comment

Looking good. Hopefully someone who knows something about cross compilation can approve. Maybe @matthewbauer?

@kisik21 kisik21 force-pushed the kisik21:master branch from 61f54e1 to 54e4228 Aug 27, 2019
@worldofpeace

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

I noticed that the pkgconfig file is handled by the Makefile now, and is currently incorrect.

This patch should fix this

diff --git a/pkgs/development/libraries/libargon2/default.nix b/pkgs/development/libraries/libargon2/default.nix
index 48c94098d58..1441aa5d112 100644
--- a/pkgs/development/libraries/libargon2/default.nix
+++ b/pkgs/development/libraries/libargon2/default.nix
@@ -20,20 +20,13 @@ stdenv.mkDerivation rec {
   ];
 
   # Fix cross-compilation
-  makeFlags = ["AR=${stdenv.cc.targetPrefix}ar"];
-
-  installPhase = ''
-    runHook preInstall
-    mkdir -p $out/lib/pkgconfig
-    substitute libargon2.pc $out/lib/pkgconfig/libargon2.pc \
-      --replace @UPSTREAM_VER@ "${version}"                 \
-      --replace @HOST_MULTIARCH@ ""                         \
-      --replace 'prefix=/usr' "prefix=$out"
-
-    make install PREFIX=$out LIBRARY_REL=lib
-    ln -s $out/lib/libargon2.so $out/lib/libargon2.so.0
-    runHook postInstall
-  '';
+  makeFlags = [
+    "AR=${stdenv.cc.targetPrefix}ar"
+    "PREFIX=${placeholder "out"}"
+    "ARGON2_VERSION=${version}"
+    "LIBRARY_REL=lib"
+    "PKGCONFIG_REL=lib"
+  ];
 
   meta = with stdenv.lib; {
     description = "A key derivation function that was selected as the winner of the Password Hashing Competition in July 2015";

Not sure if the ln -s $out/lib/libargon2.so $out/lib/libargon2.so.0 part was needed.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

For reference the file currently looks like

# libargon2 info for pkg-config

prefix=/usr
exec_prefix=${prefix}
libdir=${prefix}/lib/x86_64-linux-gnu
includedir=${prefix}/include

Name: libargon2
Description: Development libraries for libargon2
Version: ZERO
Libs: -L${libdir} -largon2 -lrt -ldl
Cflags: -I${includedir}
URL: https://github.com/P-H-C/phc-winner-argon2

I'd expect anything that uses pkgconfig for libargon2 to be broken because of that.

@kisik21 kisik21 force-pushed the kisik21:master branch from 54e4228 to abd6eaa Aug 28, 2019
@kisik21

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@worldofpeace Your patches were successfully assimilated 😸

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@worldofpeace Your patches were successfully assimilated smile_cat

Great @kisik21, could you note this change in the commit msg? Or if convenient make it a separate commit.

@kisik21

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Oops! I've force-pushed it to my commit >.< One second, I'll amend the commit message.

@kisik21 kisik21 force-pushed the kisik21:master branch from abd6eaa to b36a785 Aug 28, 2019
@kisik21

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Noted the pkg-config fix in the commit message and attributed the patch.

Copy link
Member

left a comment

Checked that it cross compiles for: aarch64-multiplatform raspberryPi.

Makefile had a hardcoded unprefixed ar. I wrote a patch (sending it
upstream) and added an optional make flag to override it in case we're
cross-compiling.

Unfortunately, this causes a rebuild of native packages.

This commit also fixes the pkg-config file to be generated correctly,
patch was provided by @worldofpeace.
@kisik21 kisik21 force-pushed the kisik21:master branch from b36a785 to 06073ae Aug 28, 2019
@worldofpeace worldofpeace merged commit 58fd72f into NixOS:master Aug 28, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.