Skip to content

haskellPackages.mkDerivation: New intermediates output#227985

Merged
cdepillabout merged 7 commits intoNixOS:haskell-updatesfrom
MercuryTechnologies:gabriella/separate_dist_2
May 25, 2023
Merged

haskellPackages.mkDerivation: New intermediates output#227985
cdepillabout merged 7 commits intoNixOS:haskell-updatesfrom
MercuryTechnologies:gabriella/separate_dist_2

Conversation

@Gabriella439
Copy link
Contributor

@Gabriella439 Gabriella439 commented Apr 24, 2023

This adds a new intermediates output that can be used to accelerate a Haskell build by importing build products exported from a prior similar build.

The motivation for this is explained by the following post:

https://www.haskellforall.com/2022/12/nixpkgs-support-for-incremental-haskell.html

This is a subset of the changes from #204020 that I believe are safe to merge while waiting for NixOS/nix#7362 to be reviewed. I've incorporated outstanding feedback from the last PR into this one.

I structured this to be a hash-preserving changes for Haskell packages that don't use the feature, so this won't trigger unnecessary rebuilds or breakage.

This is a redo of #213817 since that inadvertently subscribed too many people to the pull request discussion.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: documentation This PR adds or changes documentation labels Apr 24, 2023
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Apr 24, 2023
@ofborg ofborg bot requested a review from 9999years April 24, 2023 17:54
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 24, 2023
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Can we do btter than this? This test would succeed as well if the intermediates were just ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. We could add a check to the buildPhase to make sure that find dist/build | wc -l is positive, or something? I'm open to suggestions here.

The nice thing about incremental builds in general is that they degrade gracefully — the build works just as well without the intermediates, it's just slower.

In any case, this test does confirm that the code to copy the intermediate artifacts doesn't break the build.

Copy link
Member

@cdepillabout cdepillabout Apr 29, 2023

Choose a reason for hiding this comment

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

The nice thing about incremental builds in general is that they degrade gracefully

Yeah, I was thinking about this as well, and this is the same conclusion I came to. I couldn't think of a good way to test that the intermediates were actually being used (without doing something hacky/brittle), but this does test at least that the intermediate artifacts don't break the build.

Copy link
Member

Choose a reason for hiding this comment

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

New arguments should also be documented in the nixpkgs manual.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can find a better dir for this (under share?) that includes pname and version so there are no collisions if enableSeparateIntermediatesOutput is false and it is installed by accident.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea. I updated this to install intermediates to share/haskell/${ghc.version}/${pname}-${version}/dist. I think it might be better to key them under ${pname} rather than ${pname}-${version}, though — we wouldn't want to have an incremental build cache miss just because a version number incremented. Let me know what you think.

Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

This LGTM.

Sorry to take so long with this. Could you fix the merge conflicts? Then lets merge in.

Gabriella439 and others added 7 commits May 25, 2023 14:32
This adds a new `intermediates` output that can be used to
accelerate a Haskell build by importing build products
exported from a prior similar build.

The motivation for this is explained by the following post:

https://www.haskellforall.com/2022/12/nixpkgs-support-for-incremental-haskell.html
Co-authored-by: sternenseemann <sternenseemann@systemli.org>
@9999years 9999years force-pushed the gabriella/separate_dist_2 branch from c6e5e85 to ec2938b Compare May 25, 2023 21:37
@9999years
Copy link
Contributor

Fixed merge conflicts.

@cdepillabout
Copy link
Member

Thanks! Looking forward to see if anyone uses this in any interesting ways.

@cdepillabout cdepillabout merged commit caa65af into NixOS:haskell-updates May 25, 2023
@sternenseemann
Copy link
Member

Please remember to make to rebase the branch to have conforming commit messages next time!

@9999years 9999years deleted the gabriella/separate_dist_2 branch June 7, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants