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

haskell.compiler.ghc822Binary: propagate llvm dependency #80355

Merged
merged 1 commit into from Mar 26, 2020

Conversation

@thefloweringash
Copy link
Member

@thefloweringash thefloweringash commented Feb 17, 2020

Fixes the following error when attempting to build packages using this
compiler:

  <no location info>: error:
      Warning: Couldn't figure out LLVM version!
               Make sure you have installed LLVM 3.9

  <no location info>: error: ghc: could not execute: opt

I don't know why this worked before, but it seems an appropriate fix.

Motivation for this change

Motivation is to make ghc build on aarch64. Fixes #80176

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS (aarch64 only)
    • 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.
@vcunat
Copy link
Member

@vcunat vcunat commented Feb 17, 2020

/cc ZHF #80379

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Feb 18, 2020

cc @Ericson2314 and @matthewbauer. I think you guys use GHC on alternative architectures, so hopefully you'll know whether this is a reasonable fix.

@disassembler
Copy link
Member

@disassembler disassembler commented Feb 19, 2020

@cdepillabout should this target haskell-updates since it's haskell related?

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Feb 20, 2020

@disassembler We often ask that changes to stuff in pkgs/development/haskell-modules/ go to the haskell-updates branch, but we're not as strict about changes to pkgs/development/compilers/ghc. Let's wait and see what @Ericson2314 / @matthewbauer / @peti have to say about this PR.

That said, rebasing this on haskell-updates wouldn't be a problem for us.

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Feb 25, 2020

Looks good to me.

From past experience, not using the "blessed" llvm version is likely (but not guaranteed, hence the warning) to work, especially if using a later version of llvm. Testing this PR, I have successfully built a working ghc865 on aarch64. I think this probably means that it's ok to ignore the warning on the basis that:

  1. GHC is a fairly large and complex haskell package: The fact that we can build it is a reasonably good test of whether the GHC with mismatched llvm is functional.
  2. We are slightly distanced from the mismatched ghc+llvm because while it was used to compile ghc865, ghc865 has it's own, correct, version of llvm.

I still can't see exactly what changed/why it worked without propagating llvm before. Is it possible that the ghc822Binary was able to function because it was finding the opt and llc binaries from the buildInputs of the package it was building? That might make sense for building ghc865, which has llvm in its buildInputs, but I can't see why the hscolour build would have opt and llc in its system path if not propagated from ghc822Binary.

Let me know if there's anything else you'd like me to test with said build of ghc865.

Copy link
Member

@peti peti left a comment

I don't think that propagatedBuildInputs is the right place to add llvm. I'd prefer if those builds that need LLVM (on certain) platforms express that dependency explicitly.

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Feb 25, 2020

Looks like the other (compiled from source) ghc packages add llvm to depsBuildTarget. Would that be an appropriate place to put it for ghc822Binary too?

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Feb 25, 2020

I tried adding llvm to depsBuildTarget; it doesn't seem to work.

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Feb 25, 2020

@peti I'm not sure if I've understood correctly. Are you suggesting that packages which depend on ghc822Binary should be responsible for adding llvm to their buildInputs instead (on platforms where it's required)?

@vcunat
Copy link
Member

@vcunat vcunat commented Feb 25, 2020

Yes, that's my understanding of the comment.

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Feb 25, 2020

It's an unusual situation, in that ghc822Binary is only really there to bootstrap other GHC builds, but isn't it a bit odd to have a compiler package which can't compile anything on its own?

Is it even possible to make hscolour depend on llvm when built by ghc822Binary on aarch64?

@vcunat
Copy link
Member

@vcunat vcunat commented Feb 25, 2020

Certainly sounds possible to me.

@thefloweringash
Copy link
Member Author

@thefloweringash thefloweringash commented Feb 26, 2020

I don't think that propagatedBuildInputs is the right place to add llvm. I'd prefer if those builds that need LLVM (on certain) platforms express that dependency explicitly.

It's my understanding that the builds that need LLVM are exactly "things built with ghc822Binary on arm". On current master, for example, trying to build a basic Haskell hello world without nix:

$ cat Test.hs
main = putStrLn "Hello, World!"

$ nix run -f . haskell.compiler.ghc822Binary -c ghc -c ./Test.hs
# [ trimmed warnings ]
<no location info>: error:
    Warning: Couldn't figure out LLVM version!
             Make sure you have installed LLVM 3.9
ghc: could not execute: opt

It seems to be a runtime dependency of ghc resolved via PATH. propagatedBuildInputs doesn't exactly capture that. In general it's the kind of thing that would go in a wrapper, is that safe to to apply here?

@thefloweringash
Copy link
Member Author

@thefloweringash thefloweringash commented Mar 18, 2020

My motivation for opening this was to restore ghc on aarch64 by fixing its bootstrap ghc. Since the version of ghc in current master (8.8.x) no longer bootstraps via ghc822Binary there's not as much to be gained by fixing ghc822Binary on aarch64. Should I go ahead and close this, even though it remains broken? If not, how can we progress this?

@Thra11
Copy link
Contributor

@Thra11 Thra11 commented Mar 18, 2020

It would be a shame not to have a working aarch64 GHC in 20.03. In the absence of a more 'correct' solution, would it make sense to put this into 20.03, but not into master?

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 21, 2020

This objection is silly to me. All of the non-bootstrap GHCs have LLVM (optionally) in propagatedBuildInputs. Maybe it's good, maybe it's bad, but it's certainly consistent, and consistency is good. Let's make it consistent and then make it better.

@thefloweringash thefloweringash force-pushed the thefloweringash:ghc-aarch64-llvm branch from 4d05a7b to 1078449 Mar 21, 2020
@thefloweringash
Copy link
Member Author

@thefloweringash thefloweringash commented Mar 21, 2020

I pushed a small tweak to make it have a structure closer to the non-bootstrap compilers.

Fixes the following error when attempting to build packages using this
compiler:

  <no location info>: error:
      Warning: Couldn't figure out LLVM version!
               Make sure you have installed LLVM 3.9

  <no location info>: error: ghc: could not execute: opt
@thefloweringash thefloweringash force-pushed the thefloweringash:ghc-aarch64-llvm branch from 1078449 to 31f557c Mar 21, 2020
@peti
Copy link
Member

@peti peti commented Mar 21, 2020

All of the non-bootstrap GHCs have LLVM (optionally) in propagatedBuildInputs. Maybe it's good, maybe it's bad, but it's certainly consistent, and consistency is good. Let's make it consistent and then make it better.

I'd prefer we make it better. propagatedBuildInputs is not the right place for LLVM. If a platform requires that compiler, then the dependency should be configured in the generic builder, not in the compiler.

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Mar 21, 2020

@peti I agree that it would be great for it to be made better, but I think it is somewhat out of scope for this PR.

I think it is reasonable to merge this in as-is (since it appears to just be making things consistent), and then correct all these uses of LLVM in a future PR.

@peti
peti approved these changes Mar 22, 2020
Copy link
Member

@peti peti left a comment

OK. Let's live with the kludge. I don't like it, but I can't suggest an alternative implementation either, so I reckon I shouldn't be standing in the way.

@domenkozar has done his usual job of merging these changes already via 8c06685 without waiting for me to approve, so I guess we can close this PR since the change is now part of #83164 and will be merged into master next Friday.

@peti peti mentioned this pull request Mar 22, 2020
2 of 4 tasks complete
@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Mar 23, 2020

I guess we can close this PR since the change is now part of #83164 and will be merged into master next Friday.

I think this PR is updating ghc822Binary, while #83048 is doing the same for ghc865Binary. It looks like #83048 has already been merged into haskell-updates, but this PR still needs to be merged in.

@thefloweringash Is this correct?

@thefloweringash
Copy link
Member Author

@thefloweringash thefloweringash commented Mar 23, 2020

I think this PR is updating ghc822Binary, while #83048 is doing the same for ghc865Binary. It looks like #83048 has already been merged into haskell-updates, but this PR still needs to be merged in.

@thefloweringash Is this correct?

Yep. I've been chasing the bootstrap compiler to bring ghc back to aarch64. On current master that's ghc865Binary which is now part of haskell-updates from #83048. For release-20.03 it's ghc822Binary which is this PR. If this is merged, we should back port to 20.03.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Mar 26, 2020

@GrahamcOfBorg build haskell.compiler.ghc865

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Mar 26, 2020

This PR doesn't affect linux nor darwin, so it's safe to merge into master. In current state it can't make aarch64 worse.

@domenkozar domenkozar merged commit 00373da into NixOS:master Mar 26, 2020
17 of 18 checks passed
17 of 18 checks passed
haskell.compiler.ghc865 on aarch64-linux
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
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
haskell.compiler.ghc822Binary on aarch64-linux Success
Details
haskell.compiler.ghc822Binary on x86_64-linux Success
Details
haskell.compiler.ghc865 on x86_64-darwin Success
Details
haskell.compiler.ghc865 on x86_64-linux Success
Details
@vcunat

This comment was marked as off-topic.

@vcunat

This comment was marked as off-topic.

@vcunat
Copy link
Member

@vcunat vcunat commented Mar 28, 2020

I moved my today's (confused) comments to a better thread: #66277

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.

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