Skip to content

Update ghc 8.4.4 based tools to ghc 8.6.5 #618

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

Merged
merged 38 commits into from
May 20, 2020
Merged

Conversation

hamishmack
Copy link
Collaborator

@hamishmack hamishmack commented May 18, 2020

This description has been updated to reflect all the changes now
made in this PR.

Although the default ghc used by haskell.nix is ghc 8.6.5 many of
the tools used in haskell.nix are still built with the boot compiler
ghc 8.4.4. These include

  • haskell-nix.cabal-install
  • haskell-nix.alex
  • haskell-nix.happy

This change updates those to ghc 8.6.5 and includes materializations
for the new versions.

When cabal-install is built it is careful to disable materialization
checks on the version of itself used during the build to avoid
infinite recursion.

There was a version of nix-tools built with the boot ghc which was
only used when checkMaterialization = true. It was used for
the boot versions of alex, happy and hscolour. These have been update
to use the default (ghc 8.6.5) version of nix-tools and
checkMaterialization is forced off when they are being used to build
ghc. This means the materialization will only be checked for these
when they are built independently (they are included in the test set
via haskellNixRoots).

Three new arguments are added to default.nix:

  • defaultCompilerNixName if not specified "ghc865" is used
  • checkMaterialization makes it easier to switch on materialization checks
  • system defaults to builtins.currentSystem

This change also moves the work needed for hydra eval to the eval
system using a new evalPackages feature. This includes:

  • Fetching from git with fetchgit
  • Building scripts with runCommand and writeTextFile
  • git ls-files in cleanGit
  • running cabal v2-configure
  • copying materialized files (we are not sure why this is necessary but
    if we do not cp -r the files nix will not load them on hydra)

Reduce size of make-config-files.nix strings by around 80%.
These are unlikely to be the cause of hydra eval time memory
issues in the GB range, but were still quite large (around 10MB for the
cabal-simple test case).

There was issue causing excessive builds of the git package when
cross compiling. Gory details are a comment in lib/defaults.nix
but in short if you use git you need an extra .buildPackages
one is not enough because it depends on gdb and that will
be different in buildPackages compared to
buildPackages.buildPackages.

Adds missing materialization files for ghc 8.4.4 (only needed
when checkMaterialization is on because of other
materialiazations, but good to have).

Currently we do not give components a version and this is inconvenient
as it means nix code that checks for a version does not work
with haskell.nix components.
Although the default ghc used by haskell.nix is ghc 8.6.5 many of
the tools used in haskell.nix are still built with the boot compiler
ghc 8.4.4.  These include

  * haskell-nix.cabal-install
  * haskell-nix.alex
  * haskell-nix.happy

This change updates those to ghc 8.6.5 and includes materializations
for the new versions.

When cabal-install is built it is careful to disable materialization
checks on the version of itself used during the build to avoid
infinite recursion.

There was a version of nix-tools built with the boot ghc which was
only used when `checkMaterialization = true`.  It was used for
the boot versions of alex, happy and hscolour.  These have been update
to use the default (ghc 8.6.5) version of nix-tools and
checkMaterialization is forced off when they are being used to build
ghc.  This means the materialization will only be checked for these
when they are built independently (they are included in the test set
via haskellNixRoots).
@michaelpj
Copy link
Collaborator

Random remark: I recommend setting linguist-generated in the gitattributes file for the materialized files.

@hamishmack hamishmack requested a review from angerman May 18, 2020 13:19
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I don't really understand what's going on with the boot packages, but seems plausible.

@@ -34,7 +34,9 @@ in {
compiler =
let bootPkgs = with final.buildPackages; {
ghc = buildPackages.haskell-nix.bootstrap.compiler.ghc844;
inherit (final.haskell-nix.bootstrap.packages) alex happy hscolour;
alex = final.haskell-nix.bootstrap.packages.alex-tool false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're here... should happy and alex also come from buildPackages?

cabal-install =
# Make sure that `cabal-install` used building `cabal-install`
# always has `checkMaterialization = false` to avoid infinite
# recursion.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could do with some expansion: I suspect figuring out why this was a problem took you some time, worth recording so future people don't have to do that!

materialized = ../materialized/cabal-install;
} // final.lib.optionalAttrs (!check) {
checkMaterialization = false;
})) // { version = "3.2.0.0"; };
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tacked the missing version number on to the component as #615 is still building on hydra. It was going to be needed to work around the assert in cabal-cabal-project-to-nix.nix that I had to remove in the end anyway (because it was too strict and was another source of infinite recursion during checkMaterialization = true). So we can safely remove it now anyway.

@michaelpj
Copy link
Collaborator

For the local stuff, shouldn't it be enough to use buildPackages.writeText or whatever?

You may also want to use runCommandLocal - although I'm a bit worried since that sets allowSubstitutes false, so I don't know what will happen if you're evaluating on something other than the build platform. (But writeTextFile does the same thing and that seems to be okay 🤷 )

@michaelpj
Copy link
Collaborator

I'm generally not super keen on relying on builtins.currentSystem if we can avoid it.

@michaelpj
Copy link
Collaborator

e.g. you won't be able to do builtins.currentSystem in a flake in future: https://github.com/NixOS/rfcs/pull/49/files#diff-a5a138ca225433534de8d260f225fe31R429

@hamishmack
Copy link
Collaborator Author

I did try pkgs.runCommand with preferLocalBuild = true; and it did not seem to work. On my darwin system with the linux builder disabled I get:

~/iohk/haskell.nix-master$ nix-build release.nix -A R2003.linux.native.tests.cabal-simple.run --show-trace
trace: Using latest index state for source-test-cabal-simple!
...
while evaluating the attribute 'pkgs' at /Users/hamish/iohk/haskell.nix-master/lib/import-and-filter-project.nix:10:3:
a 'x86_64-linux' is required to build '/nix/store/2kmxa9ha8qxn3pja7ws84z586v3l8rw2-source-test-cabal-simple-plan-to-nix-pkgs.drv', but I am a 'x86_64-darwin'

@cleverca22 suggested (import pkgs.path {}).runCommand which is what it does not (I presume that is using the builtins.currentSystem default for system.

@hamishmack
Copy link
Collaborator Author

I guess if we forward all the local system stuff via final.haskell-nix.currentSystemPkgs then we can set that up in an overlay and leave it out of the flake.

@hamishmack
Copy link
Collaborator Author

Well we can set currentSystemPkgs = final; in the flake version of the overlay.

@michaelpj
Copy link
Collaborator

Hm, how about calling it evalPackages then? With the meaning being "these are the pkgs we use for eval-time stuff. Then in the flake version we'll just have to set it to the build packages, which is technically wrong but 🤷

Maybe we should have an evalSystem argument like crossSystem? Is that crazy?

Copy link
Collaborator

@angerman angerman left a comment

Choose a reason for hiding this comment

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

Nothing looks obviously off.

@hamishmack
Copy link
Collaborator Author

Hm, how about calling it evalPackages then?

Much nicer name. I have updated the code and made two overlays (eval-on-current and eval-on-build for use in the flake). I put evalPackages at the top level (so it is along side buildPackages I am little worried it might clash in the future with something nixpkgs does, but I think it easy enough to adapt.

Maybe we should have an evalSystem argument like crossSystem? Is that crazy?

I like this idea, but perhaps this bit should be done in nixpkgs (or at least in another PR as this one is already quite big).

@michaelpj
Copy link
Collaborator

Again probably not for this PR, but I find the use of evalPackages throughout a bit tricky, typically because we're using it at the same time as other package sets. And e.g. it's not always true that we should use it for runCommand, it depends on whether the output gets used for IFD.

I wonder if we could profit from more thoroughly separating the eval-time stuff from the build time stuff and using evalPackages.callPackage etc. to make it clear that "this whole thing is done at eval time".

@michaelpj
Copy link
Collaborator

e.g. I think we could pull https://github.com/input-output-hk/haskell.nix/pull/618/files#diff-9f18a53e9ed9b1e7204e2dd3110e00e0R305 into its own file, move the dummy-ghc stuff in there, and then do evalPackages.callPackage on the whole thing.

@hamishmack hamishmack marked this pull request as ready for review May 20, 2020 22:57
@hamishmack hamishmack merged commit 099d830 into master May 20, 2020
@iohk-bors iohk-bors bot deleted the hkm/ghc844-to-ghc865 branch May 20, 2020 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants