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

[RFC] A large batch of cross-compilation fixes #30882

Closed
wants to merge 164 commits into from

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Oct 28, 2017

Note: The prefix of commits authored by @Ericson2314 can be ignored for the purposes of this pull request. These are prerequisites for this work but I'll leave upstreaming these to @Ericson2314.

Motivation for this change

Allow Nix to cross-compile a basic system environment.

Things done

The only testing I have done thusfar is to build systemd and its dependencies with the following overlay:

self: super:                                                                                                                                                                                                                                                                                                                 
                                                                                                                                                                                                                                                                                                                             
let                                                                                                                                                                                                                                                                                                                          
  pkgs = self.pkgs;                                                                                                                                                                                                                                                                                                          
  callPackage = self.pkgs.callPackage;                                                                                                                                                                                                                                                                                       
in rec {                                                                                                                                                                                                                                                                                                                     
  # guile 2.2 doesn't cross compile. See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28920                                                                                                                                                                                                                                 
  guile = super.guile_2_0;                                                                                                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                                                                                             
  # We really don't need libapparmor and it is a pain to cross compile due to                                                                                                                                                                                                                                                
  # its perl bindings                                                                                                                                                                                                                                                                                                        
  systemd = super.systemd.override { libapparmor = null; };                                                                                                                                                                                                                                                                  
}                                                                                                 

That being said, this was no small feat. I would like to start getting pieces of this work upstream but I'm a bit uncertain as to how to go about this as there are a few open questions. Namely,

  1. Are we okay with continuing to pepper expressions with doCheck = stdenv.buildPlatform == stdenv.hostPlatform?
  2. Why have I needed to add so many buildPackages. to nativeBuildInputs? Shouldn't this be redundant? Is there perhaps a bug, @Ericson2314?
  3. How do we guarantee that things like fetchurl always use build packages? Is the approach I took here acceptable? It seems something similar will need to be done with unpackCmd.
  4. Is the Always search DT_RPATH hack acceptable?
  5. Some packages' configure scripts have tests that want to run host programs on the build machine. This obviously isn't possible so I provide these test results manually (see, for instance, the krb5 expression). These are of course guesses, but I believe they should apply in most sensible environments. Is this acceptable?

Whatever reviews you can offer are greatly appreciated but I really just wanted to raise a flag so people know this work exists.

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

@bgamari
Copy link
Contributor Author

bgamari commented Oct 28, 2017

I have also opened #30883 which isolates the (hopefully reasonably uncontroversial) doCheck changes.

@zimbatm
Copy link
Member

zimbatm commented Nov 5, 2017

Wow, this is impressive. Is there any way to cut the PR in smaller manageable chunks?

Also, how do I test this? The nixpkgs has some notes on cross-compilation but it's not really clear how to trigger it.

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 5, 2017

  1. Once the other PRs contained within here are merged, the changes are fairly self-similar so I don't think breaking up is too important.

  2. I was told at NixCon to provide some easy instantiatations for that.

@@ -15,7 +15,7 @@ stdenv.mkDerivation rec {
buildInputs = [ libnetfilter_conntrack libnftnl libmnl ];

preConfigure = ''
export NIX_LDFLAGS="$NIX_LDFLAGS -lmnl -lnftnl"
export NIX_LDFLAGS="$NIX_LDFLAGS -lmnl -lnftnl -ldl"
'';
Copy link
Member

Choose a reason for hiding this comment

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

commit message typo, libld -> libdl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed in my local branch.

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 5, 2017

@zimbatm buried within section 5.3 is

nix-build <nixpkgs> --arg crossSystem '(import <nixpkgs/lib>).systems.examples.fooBarBaz

A concrete example would be

nix-instantiate --arg crossSystem '(import ./lib).systems.examples.aarch64-multiplatform' -A systemd

until I do that above that should test this.

@Ericson2314 Ericson2314 added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Nov 5, 2017
@bgamari
Copy link
Contributor Author

bgamari commented Nov 5, 2017

Wow, this is impressive. Is there any way to cut the PR in smaller manageable chunks?

As @Ericson2314 has said, many of the changes are quite similar. That being said, some are not and deserve closer consideration. I can certainly break these out.

@zimbatm
Copy link
Member

zimbatm commented Nov 6, 2017

Is there any way to make doCheck = stdenv.buildPlatform == stdenv.hostPlatform automatic? I guess it would require to change the builder.sh. I think that it's too much to ask for normal packagers to remember cross-compilation, we are going to get regressions all the time otherwise.

''

+ ''
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not a good candidate for a dedicated PR on top of #29396 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so.

@@ -36,8 +38,11 @@ stdenv.mkDerivation rec {
mkdir -p $out/lib
cp src/.libs/libgcrypt.20.dylib $out/lib
'';
configureFlags = [
''--with-libgpg-error-prefix=${libgpgerror.dev.outPath}''
];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this works at all. libgcrypt/libgpgerror produce some asm code during configure by producing binaries according to the machine architecture. I know these issues from my experiments with webassembly, which is similar to cross-compilation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a look at the git repo of libgcrypt and i meant libgcrypt/mpi in my previous comment.

"AR=ar"
"RANLIB=ranlib"
"OBJCOPY=objcopy"
"CC=${stdenv.cc.prefix}gcc"
Copy link
Contributor

@periklis periklis Nov 6, 2017

Choose a reason for hiding this comment

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

Shouldn't this be cc instead of gcc to be more clang-friendly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it should.

# However, some packages (libssh2) build non-installed executables
# in their build trees which link against shared libraries
# in their tree. Linking these breaks if we are so restrictive.
*/*.so | */*.so.*)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, shouldn't this be a separate PR by its own on top of #29396 ?

# shared objects when cross-compiling. Consequently, we are forced to
# override this behavior, forcing ld to search DT_RPATH even when
# cross-compiling.
./always-search-rpath.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, shouldn't this be a separate PR on top of #29396 ?

propagatedBuildInputs = propagatedBuildInputs ++ buildInputs;
propagatedBuildInputs =
propagatedBuildInputs ++ buildInputs ++ __depsBuildTargetPropagated ++ __depsHostHostPropagated ++ __depsTargetTarget ++
depsBuildBuild ++ depsBuildTarget ++ __depsHostHost ++ __depsBuildBuildPropagated ++ __depsTargetTargetPropagated;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, shouldn't this be a separate PR on top of #29396 ?

@@ -49,7 +49,7 @@ let
];

buildInputs = [
perl
#perl
Copy link
Contributor

Choose a reason for hiding this comment

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

why not drop this comment here? Imho the --without-perl below is quite self-explanatory.

@periklis
Copy link
Contributor

periklis commented Nov 6, 2017

First of all many many kudos to @bgamari for this batch of insightful work.

@Ericson2314 & @bgamari
I think this PR hits the surface in terms of cross compilation. These changes make visible what work has already been done and what cc patterns for maintainers emerge. We will need a docs/education/migration step for packages to use the cross-compilation pipeline after #29396 and #26805 get merged beyond just explaining all the derivation inputs variables.

I noticed the following patterns for maintainers, please beat me if i misunderstood:

  • Package maintainers have to adapt their doCheck to assert host==build or blacklist tests not cross-compiling
  • Package maintainers have to split their nativeBuildInputs and nativePropagatedBuildInputs because of the minimum dependency for buildPackages.stdenv.cc in the first variable
  • Package maintainers of Autoconf-based packages need to be aware of the autoreconfHook
  • Package maintainers of packages with assembler-generating configure code should be aware of how to manage their build process for cc, e.g. libgcrypt/mpi
  • Package maintainers need to be aware when to use configurePlatforms, e.g. guile
  • Educate maintainers not to hardcode usage of tools from the GNU Toolchain, eg. ar

Also the following hacks are not obvious from a maintenance perspective, but work:

Finally, the following list of packages mentioned in this PR-changeset, should be picked in a separate PR in order to provide a minimum proper cross-compiling base set for package maintainers, in case this PR takes to long to hit master/staging:

P.S. Yes i went through each commit :)

@zimbatm
Copy link
Member

zimbatm commented Nov 6, 2017

nice review @periklis!

Would it make sense to start with a simple package like hello before touching so many files?

@periklis
Copy link
Contributor

periklis commented Nov 6, 2017

@zimbatm afaik nixpkgs.hello is not representative enough to hit so many important missing parts as @bgamari did with systemd. Imho we need repsentative closures for people that cross-compile by purpose, e.g.

  • the ARM-stuffs needs most parts of a running linux system
  • the Darwin->linux stuffs for docker images needs mostly some libc and build inputs of the final package.

@bgamari
Copy link
Contributor Author

bgamari commented Nov 6, 2017

Thanks for the review @periklis! I'll split this up a bit more in the next few days.

@Ericson2314
Copy link
Member

I sort of mentioned this to @bgamari on IRC, but let me reiterate here.

Package maintainers have to split their nativeBuildInputs and nativePropagatedBuildInputs because of the minimum dependency for buildPackages.stdenv.cc in the first variable

  1. The propagated variable is actually propagatedNativeBuildInputs.
  2. buildPackages.stdenv.cc should always be a depsBuildBuild (that wouldn't be propagated).
  3. Currently, there are very few uses of buildPackages.stdenv.cc, just what I and others recently added. They are all nativeBuildInputs as a stop-gap, it is fairly easy to grep and make them depsBuildBuild as I did.

@periklis
Copy link
Contributor

periklis commented Nov 7, 2017

@Ericson2314 Thx for re-iterating the information on IRC. What do you think about the rest of the patterns, i identified above? I would be glad to know your opinion on that. Maybe these are too minor to bother.

bgamari and others added 22 commits February 27, 2018 12:41
It crashes with gcc7.
Otherwise building this fails with -Werror warnings when bootstrapping from
gcc7.
Otherwise gems tries to delete files for the build ruby.
This provides fw_printenv and fw_setenv, which are used to manipulate
the bootloader configuration on some platforms.
The ipsec utility provided by strongswan is a shell script. Consequently we need
to patch it to use the target's shell.
The strongswan-swanctl systemd service starts charon-systemd. This implements a IKE daemon
very similar to charon, but it's specifically designed for use with systemd. It uses the
systemd libraries for a native integration.

Instead of using starter and an ipsec.conf based configuration, the daemon is directly
managed by systemd and configured with the swanctl configuration backend.

See: https://wiki.strongswan.org/projects/strongswan/wiki/Charon-systemd

Note that the strongswan.conf and swantctl.conf configuration files are automatically
generated based on NixOS options under services.strongswan-swanctl.strongswan and
services.strongswan-swanctl.swanctl respectively.
I determined which options got changed by executing the following
commands in the strongswan repository:

  git diff -U20 5.6.0..5.6.1 src/swanctl/swanctl.opt
  git diff -U20 5.6.0..5.6.1 conf
This reduces the number of options from 1152 to 756.
@dtzWill
Copy link
Member

dtzWill commented May 18, 2018

Just curious, are there still pieces of this that need to be "upstreamed"?

@bgamari
Copy link
Contributor Author

bgamari commented May 20, 2018

There are a few more patches, but they really aren't upstreamable in their current form. Perl still remains the largest issue (#36675). I have reverted the commit cited in that ticket on my branch.

Unfortunately I have been unable to build my rebased branch recently due to an inexplicable stack overflow in the nix interpreter. I'll need to sit down and try to debug this some day.

@Ericson2314 Ericson2314 added this to the 19.03 milestone Aug 26, 2018
@FRidh
Copy link
Member

FRidh commented Jan 6, 2019

A lot of cross-related changes have been merged since this PR. Because this can't be merged in this state anyway I am closing this now. I think it's better to pick the things that are mergeable and open smaller PR's for them.

@FRidh FRidh closed this Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: fetch 6.topic: haskell 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: ruby 6.topic: vim 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/`
Projects
None yet
Development

Successfully merging this pull request may close these issues.