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

stdenv, haskell: bonafied GHCJS cross compilation without stdenv.cc #74090

Merged
merged 23 commits into from Dec 31, 2019

Conversation

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 25, 2019

Motivation for this change

This commit allows treating GHCJS builds as cross compilation. This is useful for a number of correctness reasons, chiefly getting the right build-time build-tool-depends so things like literate READMEs work.

There is some trickiness to get this to work however. Read the first commit for details. In short, as usual, dependency "splicing" is a terrible idea but one with no better alternative that isn't a huge refactor.

I was very careful to break any interfaces with this e.g. existing native and cross platforms will have the same hashes, and whether stdenv.cc == null before, that is still the case. This will allow me to backport this to 19.09 for e.g. reflex-platform and haskell.nix.

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 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 @ElvishJerricco @ElvishJerricco @ali-abrar @cdepillabout

Ericson2314 added 3 commits Nov 24, 2019
Before, we'd always use `cc = null`, and check for that. The problem is
this breaks for cross compilation to platforms that don't support a C
compiler.

It's a very subtle issue. One might think there is no problem because we
have `stdenvNoCC`, and presumably one would only build derivations that
use that. The problem is that one still wants to use tools at build-time
that are themselves built with a C compiler, and those are gotten via
"splicing". The runtime version of those deps will explode, but the
build time / `buildPackages` versions of those deps will be fine, and
splicing attempts to work this by using `builtins.tryEval` to filter out
any broken "higher priority" packages (runtime is the default and
highest priority) so that both `foo` and `foo.nativeDrv` works.

However, `tryEval` only catches certain evaluation failures (e.g.
exceptions), and not arbitrary failures (such as `cc.attr` when `cc` is
null). This means `tryEval` fails to let us use our build time deps, and
everything comes apart.

The right solution is, as usually, to get rid of splicing. Or, baring
that, to make it so `foo` never works and one has to explicitly do
`foo.*`. But that is a much larger change, and certaily one unsuitable
to be backported to stable.

Given that, we instead make an exception-throwing `cc` attribute, and
create a `hasCC` attribute for those derivations which wish to
condtionally use a C compiler: instead of doing `stdenv.cc or null ==
null` or something similar, one does `stdenv.hasCC`. This allows quering
without "tripping" the exception, while also allowing `tryEval` to work.

No platform without a C compiler is yet wired up by default. That will
be done in a following commit.
This platform doesn't have a C compiler, and so relies and the changes
in the previous commit to work.
This is GHCJS, and perhaps other obscure targets.
@Ericson2314 Ericson2314 requested review from matthewbauer and dmjio Nov 25, 2019
@@ -144,7 +144,7 @@ let
substituteInPlace "$out"/lib/perl5/*/*/Config_heavy.pl \
--replace "${libcInc}" /no-such-path \
--replace "${
if stdenv.cc.cc or null != null then stdenv.cc.cc else "/no-such-path"
if stdenv.hasCC then stdenv.cc.cc else "/no-such-path"

This comment has been minimized.

@ElvishJerricco

ElvishJerricco Nov 25, 2019 Contributor

Why not just optionalString the whole --replace argument?

This comment has been minimized.

@Ericson2314

Ericson2314 Nov 25, 2019 Author Member

I agree eventually, but trying to avoid a mass rebuild for now :)

Ericson2314 added 2 commits Nov 25, 2019
js-ghcjs didn't fit in an existing categor.
Ericson2314 added 2 commits Nov 25, 2019
js-ghcjs didn't fit in an existing categor.
…into ghcjs-cross-without-cc
@matthewbauer
Copy link
Member

matthewbauer commented Nov 27, 2019

Can you add at least one test for release-cross.nix as well?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Nov 28, 2019

Great idea

Ericson2314 added 3 commits Dec 24, 2019
Use `buildPackages.stdenv.mkDerivation` because we are making a shell
script to start hoogle on the build platform.
@Ericson2314
Copy link
Member Author

Ericson2314 commented Dec 25, 2019

I added it, but it won't work yet (at least on 19.09) because ghcjs itself doesn't build. I think that's fine; I rather upstream whatever reflex-platform does to fix it after this change.

Ericson2314 and others added 2 commits Dec 25, 2019
This is a bit dubvious, but the alternative of making nodejs a
nativeBuildInput for node packages is worse. In general the cross story
for interpreted languages is murky, and this fits that pattern.
This makes us a bit more robust to various splicing nastiness. May splicing
someday go so we don't have to resort to such hacks.
@Ericson2314 Ericson2314 changed the title stdenv, haskell: GHCJS cross compilation without CC stdenv, haskell: bonafied GHCJS cross compilation Dec 26, 2019
@Ericson2314 Ericson2314 changed the title stdenv, haskell: bonafied GHCJS cross compilation stdenv, haskell: bonafied GHCJS cross compilation withou stdenv.cc Dec 26, 2019
@Ericson2314 Ericson2314 changed the title stdenv, haskell: bonafied GHCJS cross compilation withou stdenv.cc stdenv, haskell: bonafied GHCJS cross compilation without stdenv.cc Dec 26, 2019
@Ericson2314 Ericson2314 changed the title stdenv, haskell: bonafied GHCJS cross compilation without stdenv.cc stdenv, haskell: bonafied GHCJS cross compilation without stdenv.cc for 19.09 Dec 30, 2019
@Ericson2314 Ericson2314 changed the title stdenv, haskell: bonafied GHCJS cross compilation without stdenv.cc for 19.09 stdenv, haskell: bonafied GHCJS cross compilation without stdenv.cc Dec 30, 2019
pkgs/stdenv/booter.nix Outdated Show resolved Hide resolved
@@ -156,7 +160,9 @@ let
"--libsubdir=\\$abi/\\$libname"
(optionalString enableSeparateDataOutput "--datadir=$data/share/${ghc.name}")
(optionalString enableSeparateDocOutput "--docdir=${docdir "$doc"}")
] ++ optionals stdenv.hasCC [

This comment has been minimized.

@matthewbauer

matthewbauer Dec 30, 2019 Member

This is a little weird considering haskell/cabal#6466. Isn't --with-gcc always needed for GHC?

This comment has been minimized.

@Ericson2314

Ericson2314 Dec 30, 2019 Author Member

I suppose but those "cross flags" do that. We should really just combine the list of flags and make cross and native more similar.

@Ericson2314 Ericson2314 merged commit cfd0138 into NixOS:master Dec 31, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
arrow-cpp, stdenv on aarch64-linux Success
Details
arrow-cpp, stdenv on x86_64-darwin Success
Details
arrow-cpp, stdenv on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
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
@Ericson2314 Ericson2314 deleted the obsidiansystems:ghcjs-cross-without-cc branch Dec 31, 2019
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

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