Navigation Menu

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

Jump to bottom

libargon2: fixed cross-compilation #67490

Merged
merged 1 commit into from Aug 28, 2019
Merged

libargon2: fixed cross-compilation #67490

merged 1 commit into from Aug 28, 2019

Conversation

vikanezrimaya
Copy link
Member

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
Copy link
Member

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

@vikanezrimaya vikanezrimaya mentioned this pull request Aug 26, 2019
Merged
@vikanezrimaya
Copy link
Member Author

@aanderse Done! P-H-C/phc-winner-argon2#277

aanderse
aanderse requested changes Aug 27, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

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
@vikanezrimaya
Copy link
Member Author

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

@vikanezrimaya
Copy link
Member Author

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

aanderse
aanderse approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

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

@worldofpeace
Copy link
Contributor

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
Copy link
Contributor

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.

@vikanezrimaya
Copy link
Member Author

@worldofpeace Your patches were successfully assimilated 😸

@worldofpeace
Copy link
Contributor

@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.

@vikanezrimaya
Copy link
Member Author

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

@vikanezrimaya
Copy link
Member Author

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

worldofpeace
worldofpeace approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

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

libargon2: fixed cross-compilation
06073ae 
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.
@worldofpeace worldofpeace merged commit 58fd72f into NixOS:master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@worldofpeace worldofpeace worldofpeace approved these changes

@aanderse aanderse aanderse approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vikanezrimaya @aanderse @worldofpeace @matthewbauer