cpupower, pciutils: enable cross compiling #34312

lopsided98 · 2018-01-27T01:36:34Z

Motivation for this change

This PR fixes cross compiling for cpupower and pciutils (a dependency of cpupower).

Things done

This PR uses some different flags for pciutils than the commit that was dropped from #34173. Of particular importance is the HOST variable, which has to be set correctly for aarch64 cross compiling to work. Coincidentally, the format of this variable is the same as the value of stdenv.hostPlatform.system for most architectures and operating systems, (Linux, Darwin, x86_64, aarch64 and armv7l, although it looks like the cpu name should be i386 for i686).

I have tested these changes natively on x86_64 and aarch64, as well as cross compiling for armv7l and aarch64.

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.

dtzWill · 2018-01-28T00:19:25Z

LGTM, works for x86_64-musl, cross-x86_64-musl, aarch-64-musl and armv6l-musl! Thanks!

adisbladis · 2018-01-28T02:27:38Z

While this looks perfectly reasonable to me it causes a large rebuild. Could you please change the target branch to staging and rebase?
Thanks!

dtzWill

Minor fixups on cpupower.

Also, seems to have reference to build-time bash:

$ nix why-depends --all ./result/ /nix/store/0w5g7g0ddn0yqb1mjfnhdsnh0lg2mcn4-bash-4.4-p12
/nix/store/jkj7l0b0jn2z4mgavnh99qryamifxhis-cpupower-4.9.78-aarch64-unknown-linux-musl
╚═══bin/cpufreq-bench_plot.sh: …#!/nix/store/0w5g7g0ddn0yqb1mjfnhdsnh0lg2mcn4-bash-4.4-p12/bin/bash..# This p…
    share/doc/cpupower/cpufreq-bench_script.sh: …#!/nix/store/0w5g7g0ddn0yqb1mjfnhdsnh0lg2mcn4-bash-4.4-p12/bin/bash..# This p…
    => /nix/store/0w5g7g0ddn0yqb1mjfnhdsnh0lg2mcn4-bash-4.4-p12

Not a big deal (script doesn't seem critical but might be good to either patch it or remove it to keep the closure neat).

dtzWill · 2018-01-28T03:04:38Z

pkgs/os-specific/linux/cpupower/default.nix


 stdenv.mkDerivation {
  name = "cpupower-${kernel.version}";

  src = kernel.src;

-  buildInputs = [ coreutils pciutils gettext ];
+  nativeBuildInputs = [ buildPackages.gettext ];


Drop buildPackages. here, just gettext.

dtzWill · 2018-01-28T03:06:13Z

pkgs/os-specific/linux/cpupower/default.nix


 stdenv.mkDerivation {
  name = "cpupower-${kernel.version}";

  src = kernel.src;

-  buildInputs = [ coreutils pciutils gettext ];
+  nativeBuildInputs = [ buildPackages.gettext ];
+  buildInputs = [ coreutils pciutils ];


coreutils doesn't seem to be needed here? Seems to build fine when dropping it, either way output has no references to it.

It's also part of stdenv.

dtzWill · 2018-01-28T03:06:52Z

pkgs/os-specific/linux/cpupower/default.nix

-  buildPhase = ''
-    make
-  '';
+  makeFlags = "CROSS=${stdenv.cc.targetPrefix}";


makeFlags "should" be an array, I think?

lopsided98 · 2018-01-28T23:45:43Z

@dtzWill The reference to bash is being added by patchShebangs:

patching script interpreter paths in /nix/store/09gkqc8bjh2cw9rbkf2jw47fnn30shv3-cpupower-4.9.78-aarch64-unknown-linux-gnu
/nix/store/09gkqc8bjh2cw9rbkf2jw47fnn30shv3-cpupower-4.9.78-aarch64-unknown-linux-gnu/bin/cpufreq-bench_plot.sh: interpreter directive changed from "/bin/bash" to "/nix/store/i0ay05pqkbnvpfijm52mmlrp6kmkl80c-bash-4.4-p12/bin/bash"
/nix/store/09gkqc8bjh2cw9rbkf2jw47fnn30shv3-cpupower-4.9.78-aarch64-unknown-linux-gnu/share/doc/cpupower/cpufreq-bench_script.sh: interpreter directive changed from "/bin/bash" to "/nix/store/i0ay05pqkbnvpfijm52mmlrp6kmkl80c-bash-4.4-p12/bin/bash"

It seems to me like patchShebangs doesn't do the right thing when cross compiling.

Ericson2314 · 2018-01-29T22:59:55Z

pkgs/os-specific/linux/cpupower/default.nix

-  buildPhase = ''
-    make
-  '';
+  makeFlags = [ "CROSS=${stdenv.cc.targetPrefix}" ];


This shouldn't be stdenv.hostPlatform.config, right? (The practical difference is the trailing -, the conceptual difference is what I wrote identifies the platform, whereas that is just the disambiguating prefix of the compiler and need not be anything in particular.

The value of CROSS is used as the compiler and linker prefix in the Makefile (see here), so isn't targetPrefix the right value for that?

Yeah sounds good

Ericson2314 · 2018-01-29T23:01:20Z

pkgs/tools/system/pciutils/default.nix

@@ -11,7 +11,7 @@ stdenv.mkDerivation rec {
  nativeBuildInputs = [ pkgconfig ];
  buildInputs = [ zlib kmod which ];

-  makeFlags = "SHARED=yes PREFIX=\${out}";
+  makeFlags = [ "SHARED=yes" "PREFIX=\${out}" "STRIP=" "HOST=${stdenv.hostPlatform.system}" "CROSS_COMPILE=${stdenv.cc.targetPrefix}" ];


Uh I don't think this alone is enough? @dtzWill and I ran into problems before with make running ./lib/Configure without any arguments.

What kind of problems? It builds fine for me and @dtzWill tested it as well.

I suppose we tried fewer arguments there; maybe these are sufficient.

Last thing, normally HOST=${stdenv.hostPlatform.config} would be better. Are you sure the shorter nix-style names are superior?

The HOST variable is parsed using custom code in the configure script (see here), so it does not follow a standard format (such as the LLVM target triple). When it is not manually specified, it is pieced together from uname -s, uname -r and a number of other commands depending on the OS. It just so happens that it matches the value of stdenv.hostPlatform.config for most systems. The only exception I can see is that when stdenv.hostPlatform.config == "i686-linux", the value of HOST should be i386-linux (see line 28), so this may break i686 builds, but I don't have a way to test this.

Cool, thanks for the diligent research! If you put that in a comment, this is fine with me. We just need a patchShebangs solution then?

dtzWill · 2018-01-29T23:47:20Z

Regarding patchShebangs, cc #33956.

dtzWill · 2018-02-04T16:47:27Z

I'm not sure we need to block this on fixing the patchShebangs situation, since that is a problem in many packages.

Ericson2314 · 2018-02-04T16:51:28Z

Oh ok.

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 501+ labels Jan 27, 2018

dtzWill requested changes Jan 28, 2018

View reviewed changes

lopsided98 added 2 commits January 28, 2018 18:47

pciutils: enable cross compilation

259b9ff

cpupower: enable cross compilation

84f54b8

lopsided98 force-pushed the cpupower-pciutils-cross branch from 0dca853 to 84f54b8 Compare January 28, 2018 23:47

lopsided98 requested review from Ericson2314, FRidh and ttuegel as code owners January 28, 2018 23:47

lopsided98 changed the base branch from master to staging January 28, 2018 23:48

GrahamcOfBorg added 10.rebuild-darwin-stdenv 10.rebuild-linux-stdenv 10.rebuild-darwin: 501+ and removed 10.rebuild-darwin: 1-10 labels Jan 28, 2018

FRidh removed their request for review January 29, 2018 14:16

Ericson2314 added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Jan 29, 2018

Ericson2314 reviewed Jan 29, 2018

View reviewed changes

Ericson2314 added this to @bgamari's numerous fixes in Cross compilation Jan 31, 2018

Ericson2314 merged commit de243f2 into NixOS:staging Feb 4, 2018

lopsided98 deleted the cpupower-pciutils-cross branch February 16, 2018 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpupower, pciutils: enable cross compiling #34312

cpupower, pciutils: enable cross compiling #34312

lopsided98 commented Jan 27, 2018

dtzWill commented Jan 28, 2018

adisbladis commented Jan 28, 2018

dtzWill left a comment

dtzWill Jan 28, 2018

dtzWill Jan 28, 2018

fpletz Jan 28, 2018

dtzWill Jan 28, 2018

lopsided98 commented Jan 28, 2018

Ericson2314 Jan 29, 2018

lopsided98 Jan 30, 2018

Ericson2314 Jan 30, 2018

Ericson2314 Jan 29, 2018

lopsided98 Jan 30, 2018

Ericson2314 Jan 30, 2018

Ericson2314 Jan 30, 2018

lopsided98 Jan 30, 2018

Ericson2314 Jan 30, 2018 •

edited

dtzWill commented Jan 29, 2018

dtzWill commented Feb 4, 2018

Ericson2314 commented Feb 4, 2018

cpupower, pciutils: enable cross compiling #34312

cpupower, pciutils: enable cross compiling #34312

Conversation

lopsided98 commented Jan 27, 2018

Motivation for this change

Things done

dtzWill commented Jan 28, 2018

adisbladis commented Jan 28, 2018

dtzWill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopsided98 commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Jan 30, 2018 • edited

Choose a reason for hiding this comment

dtzWill commented Jan 29, 2018

dtzWill commented Feb 4, 2018

Ericson2314 commented Feb 4, 2018

Ericson2314 Jan 30, 2018 •

edited