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: Fix cross platform build #97009

Closed
wants to merge 1 commit into from
Closed

netpbm: Fix cross platform build #97009

wants to merge 1 commit into from

Conversation

@kampka
Copy link
Contributor

@kampka kampka commented Sep 3, 2020

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.
@kampka
Copy link
Contributor Author

@kampka kampka commented Sep 3, 2020

@GrahamcOfBorg build netpbm
@GrahamcOfBorg build pkgsCross.aarch64-multiplatform.netpbm

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 3, 2020

I tried updating the package (#97045) if it makes any difference but unfortunately not will e-mail the maintainer.

--replace 'RANLIB = ranlib' 'RANLIB = ${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}ranlib'
substituteInPlace buildtools/install.sh \
--replace 'STRIPPROG-strip' 'STRIPPROG:-${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}strip'

This comment has been minimized.

@jtojnar

jtojnar Sep 3, 2020
Contributor

Looks like setting environment variable STRIPPROG = lib.optionalString (stdenv.buildPlatform != stdenv.hostPlatform) "${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}strip"; should work.

This comment has been minimized.

@kampka

kampka Sep 4, 2020
Author Contributor

You are right, I assumed the same. It does not work though and I haven't investigated very deeply on why that is.

substituteInPlace config.mk.in \
--replace 'AR = ar' 'AR = ${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}ar'
substituteInPlace config.mk.in \
--replace 'RANLIB = ranlib' 'RANLIB = ${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}ranlib'
Comment on lines +46 to +65

This comment has been minimized.

@jtojnar

jtojnar Sep 3, 2020
Contributor

I would just append them to the generated config.mk like we do with the rest configure flags.

This comment has been minimized.

@kampka

kampka Sep 4, 2020
Author Contributor

The difference is that the rest of the configure flags are commented out in the config.mk while AR and RANLIB are not.
It might work to just append, but I am not very familiar with the semantics here, so I opted for the most explicit solution.

This comment has been minimized.

@jtojnar

jtojnar Sep 4, 2020
Contributor

The resulting file just gets included into Makefile, so later definitions should override the previous ones.


preBuild = ''
pushd buildtools
make CC=${buildPackages.stdenv.cc}/bin/${buildPackages.stdenv.cc.targetPrefix}cc

This comment has been minimized.

@jtojnar

jtojnar Sep 3, 2020
Contributor

Could you clarify why is this needed? Would not the default CC = cc in config.mk already be the same?

nix-repl> pkgsCross.aarch64-multiplatform.buildPackages.stdenv.cc.targetPrefix       
""

This comment has been minimized.

@jtojnar

jtojnar Sep 3, 2020
Contributor

Oh, looks like during cross-compilation there are only the prefixed binaries.

Though using CC environment variable might work.

This comment has been minimized.

@kampka

kampka Sep 4, 2020
Author Contributor

During cross-platform build, the buildtools subpackage needs to be built with the local native compiler as those tools will be run during build, while the rest of the package will need to be built using the target platform compiler.
That is why I am building them separately like this.

This comment has been minimized.

@jtojnar

jtojnar Sep 4, 2020
Contributor

Right, I did not notice the directory change.

Actually, it looks like the build tools makefile uses CC_FOR_BUILD variable (defaulting to $(CC)) so setting it in config.mk should remove the necessity to invoke make manually.

This comment has been minimized.

@kampka

kampka Sep 4, 2020
Author Contributor

You are right, it should work, but the build fails for buildtools with correct CC_FOR_BUILD and LD_FOR_BUILD set in config.mk. The workaround in preBuild works, though I haven't investigated any further why config.mk doesn't.

pkgs/tools/graphics/netpbm/default.nix Outdated Show resolved Hide resolved
@kampka kampka force-pushed the kampka:netpbm branch from 9794eca to ed679ac Sep 4, 2020
@kampka
Copy link
Contributor Author

@kampka kampka commented Sep 4, 2020

@GrahamcOfBorg build netpbm
@GrahamcOfBorg build pkgsCross.aarch64-multiplatform.netpbm

postPatch = ''
# Install libnetpbm.so symlink to correct destination
substituteInPlace lib/Makefile \
--replace '/sharedlink' '/lib'
'' + (stdenv.lib.optionalString (stdenv.buildPlatform != stdenv.hostPlatform) ''
substituteInPlace buildtools/configure.pl \

This comment has been minimized.

@jtojnar

jtojnar Sep 4, 2020
Contributor

We do not use buildtools/configure.pl since it only supports interactive mode.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 4, 2020

I tried the following patch (cross by default):

diff --git a/pkgs/tools/graphics/netpbm/default.nix b/pkgs/tools/graphics/netpbm/default.nix
index 4507c4fed85..67167c8193d 100644
--- a/pkgs/tools/graphics/netpbm/default.nix
+++ b/pkgs/tools/graphics/netpbm/default.nix
@@ -12,6 +12,7 @@
 , libtiff
 , enableX11 ? false
 , libX11
+, buildPackages
 }:
 
 stdenv.mkDerivation {
@@ -48,6 +49,10 @@ stdenv.mkDerivation {
     # Install libnetpbm.so symlink to correct destination
     substituteInPlace lib/Makefile \
       --replace '/sharedlink' '/lib'
+
+    # Respect the path set in config.mk
+    substituteInPlace GNUmakefile \
+      --replace 'pkg-config' '$(PKG_CONFIG)'
   '';
 
   configurePhase = ''
@@ -66,11 +71,19 @@ stdenv.mkDerivation {
 
     # Fix path to rgb.txt
     echo "RGB_DB_PATH = $out/share/netpbm/misc/rgb.txt" >> config.mk
+
+    # For cross-compilation
+    echo "CC = ${stdenv.cc}/bin/${stdenv.cc.targetPrefix}cc" >> config.mk
+    echo "AR = ${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}ar" >> config.mk
+    echo "PKG_CONFIG = ${buildPackages.pkgconfig}/bin/${buildPackages.pkgconfig.targetPrefix}pkg-config" >> config.mk
+    echo "RANLIB = ${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}ranlib" >> config.mk
+    echo "CC_FOR_BUILD = ${buildPackages.stdenv.cc}/bin/${buildPackages.stdenv.cc.targetPrefix}cc" >> config.mk
+    echo "LD_FOR_BUILD = ${buildPackages.binutils}/bin/${buildPackages.binutils.targetPrefix}ld" >> config.mk
   '' + stdenv.lib.optionalString stdenv.isDarwin ''
     echo "LDSHLIB=-dynamiclib -install_name $out/lib/libnetpbm.\$(MAJ).dylib" >> config.mk
     echo "NETPBMLIBTYPE = dylib" >> config.mk
     echo "NETPBMLIBSUFFIX = dylib" >> config.mk
-
+  '' + ''
     runHook postConfigure
   '';
 
@@ -94,6 +107,9 @@ stdenv.mkDerivation {
     runHook postInstall
   '';
 
+  # Environment variables
+  STRIPPROG = "${stdenv.lib.getBin stdenv.cc.bintools.bintools}/bin/${stdenv.cc.targetPrefix}strip";
+
   passthru.updateScript = ./update.sh;
 
   meta = {

But it fails for native:

/nix/store/jgcv40jcr1v9bmnj070mgvyi6m7r7w5r-gcc-wrapper-9.3.0/bin/cc -c -o typegen.o  typegen.c
/nix/store/r6wg5hrz4kxcdjmjsbxn9xr55zxzpr07-binutils-wrapper-2.31.1/bin/ld -o typegen  typegen.o
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: warning: cannot find entry symbol _start; defaulting to 0000000000401000
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: typegen.o: in function `main':
typegen.c:(.text.startup+0xc): undefined reference to `puts'
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: typegen.c:(.text.startup+0x18): undefined reference to `puts'
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: typegen.c:(.text.startup+0x24): undefined reference to `puts'
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: typegen.c:(.text.startup+0x30): undefined reference to `puts'
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: typegen.c:(.text.startup+0x3c): undefined reference to `puts'
/nix/store/zp4vhfn31ky68xy0s6mssxh4c90z9v9a-binutils-2.31.1/bin/ld: typegen.o:typegen.c:(.text.startup+0x48): more undefined references to `puts' follow
make[1]: *** [/build/advanced-r3735/buildtools/Makefile:47: typegen] Error 1

and for cross:

/nix/store/jgcv40jcr1v9bmnj070mgvyi6m7r7w5r-gcc-wrapper-9.3.0/bin/cc -c -o typegen.o  typegen.c
/nix/store/0wry6m9bb5gm07s71pvf8l0cmfpay3gw-aarch64-unknown-linux-gnu-binutils-wrapper-2.31.1/bin/aarch64-unknown-linux-gnu-ld -o typegen  typegen.o
/nix/store/c178lyxkahsr67s48wc2r4ynii5q05qw-aarch64-unknown-linux-gnu-binutils-2.31.1/bin/aarch64-unknown-linux-gnu-ld: typegen.o: Relocations in generic ELF (EM: 62)
/nix/store/c178lyxkahsr67s48wc2r4ynii5q05qw-aarch64-unknown-linux-gnu-binutils-2.31.1/bin/aarch64-unknown-linux-gnu-ld: typegen.o: Relocations in generic ELF (EM: 62)
/nix/store/c178lyxkahsr67s48wc2r4ynii5q05qw-aarch64-unknown-linux-gnu-binutils-2.31.1/bin/aarch64-unknown-linux-gnu-ld: typegen.o: error adding symbols: file in wrong format
make[1]: *** [/build/advanced-r3735/buildtools/Makefile:47: typegen] Error 1
@kampka kampka force-pushed the kampka:netpbm branch from ed679ac to ada59bd Sep 6, 2020
@kampka kampka force-pushed the kampka:netpbm branch from ada59bd to 47b128c Sep 6, 2020
@kampka
Copy link
Contributor Author

@kampka kampka commented Sep 6, 2020

@jtojnar That is pretty much the same results I am getting.
So how do you want to procede with this?

@kampka
Copy link
Contributor Author

@kampka kampka commented Sep 6, 2020

@GrahamcOfBorg build netpbm
@GrahamcOfBorg build pkgsCross.aarch64-multiplatform.netpbm

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 7, 2020

Managed to get it building mostly using config.mk in #97045

@kampka
Copy link
Contributor Author

@kampka kampka commented Sep 7, 2020

Closing this in favor of #97045

@kampka kampka closed this Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.