haskell-generic-builder: fix bug when buildTarget does not depend on library by thomie · Pull Request #392228 · NixOS/nixpkgs

thomie · 2025-03-22T23:36:37Z

Proposal

Run ./Setup configure ${buildTarget} instead of ./Setup configure

Justification

Currently, the generic builder essentially runs:

  ./Setup configure
  ./Setup build ${buildTarget}
  ./Setup copy
  ./Setup register

Now consider the scenario of a haskell package containing a library and an executable (foo), but the executable does not depend on the library. See the changes in pkgs/test/haskell/setBuildTarget for an example.

In case the nix package defines buildTarget=foo, the generic builder does configure the library (since it always configures all targets), but doesn't build it (it only builds the foo executable).

This causes a problem in the register step, because Setup will try to register a library that hasn't been built, and GHC will fail with:

      Error: Setup: '/nix/store/ah2iqzixws3fkhvlml0859hqzjjrvgsz-ghc-9.8.4/bin/ghc'
      exited with an error:
      <command line>: Could not find module ‘SomeModule’.
      Use -v to see a list of the files searched for.

To fix, we change the generic builder to run:

  ./Setup configure ${buildTarget}
  ./Setup build ${buildTarget}
  ./Setup copy
  ./Setup register

Should save some cpu cycles as well.

Relevant: #120249.

~~I'm afraid it will rebuild all haskell packages. I'm not sure what to do about that.~~

Commits:

Add a new test
Demonstrate the bug
Fix the bug
~~4. Prevent rebuilding too many packages (this commit should probably not be merged?)~~

Things done

Add a 👍 reaction to pull requests you find important.

thomie · 2025-03-22T23:39:53Z

pkgs/development/haskell-modules/generic-builder.nix

packageConfFile can be an empty directory, so mv "$packageConfFile/"* "$packageConfDir" would fail.

This comment deserves to be in-code, I guess.

Do we really need to loop and mv each file separately, though?

We could also just do something like:

# Glob expansion can fail, when $packageConfFile is an empty directory. mv "$packageConfFile/"* "$packageConfDir" || true

Although, I am missing why the directory can be empty? Is this a separate bug fix?

mv "$packageConfFile/"* "$packageConfDir" || true

Hm, that would silence other errors that might occur during the mv.

How about instead of this for loop, we test whether the directory is non-empty:

if [ -d "$packageConfFile" ] && [ "$(ls -A "$packageConfFile")" ];

Although, I am missing why the directory can be empty? Is this a separate bug fix?

The directory can now be empty when buildTarget doesn't include any libraries (or targets that depend on libraries), but the package does contain at least one library (so isLibrary=true). It's not a separate bug fix. Before, the register step would already have failed in this scenario.

One could argue there is another bug in this file, and that is the existence or usage of the isLibrary variable. It's not quite clear what isLibrary means or is supposed to do when a buildTarget is set. Perhaps we should try to remove the isLibrary variable, and make sure all code works whether we're configuring/building/installing a library or not. For example, after this change set, I think we can already remove the if clause from the start of the installPhase, but I didn't try it:

${if !isLibrary && buildTarget == "" then "${setupCommand} install" # ^^ if the project is not a library, and no build target is specified, we can just use "install". else if !isLibrary then "${setupCommand} copy ${buildTarget}" # ^^ if the project is not a library, and we have a build target, then use "copy" to install # just the target specified; "install" will error here, since not all targets have been built.

thomie · 2025-03-23T11:06:22Z

pkgs/development/haskell-modules/generic-builder.nix

This ugly if/else is here to prevent rebuilding too many packages. It's from the 4th commit: "haskell-generic-build: try to prevent mass rebuild". It should probably not be merged?

Yeah, we don't need to merge the last commit, we are rebuilding everything anyway on current haskell-updates.

wolfgangwalther · 2025-03-30T19:33:37Z

pkgs/development/haskell-modules/generic-builder.nix

Hm, silently disabling the tests feels odd to me.

How about changing the default values for doCheck and doBenchmark to be false when a buildTarget is set, instead? Then, they could still be overwritten.

(It is possible to specify the build target in a way that would include the tests, right?)

Hm, silently disabling the tests feels odd to me.

Note that this change does not disable the tests, it merely disables configuring the tests with --enable-tests when a buildTarget is set.

How about changing the default values for doCheck and doBenchmark to be false when a buildTarget is set, instead? Then, they could still be overwritten.

No I don't think that works.

doCheck currently does 2 things:

configure the haskell package with --enable-tests

tell nix to run the checkPhase

Say a package sets some buildTarget (including a test-suite), and explicitly sets doCheck=True.

With your suggestion, we would still pass --enable-tests together with the buildTarget to Setup --configure, and Cabal would complain about incompatible options.

On the other hand, if the package sets the same buildTarget (including a test-suite) but doesn't override doCheck, then the test-suite would never run.

(It is possible to specify the build target in a way that would include the tests, right?)

Yes, see for example here:

nixpkgs/pkgs/development/tools/continuous-integration/hci/default.nix

Line 26 in 49ca8bc

buildTarget = "lib:hercules-ci-agent hercules-ci-agent-unit-tests";

I still don't understand what would happen in the simple case:

I rely on the default for doCheck=true.

I set a buildTarget

Are the tests run or not?

Yes they will run, but you do need to list the tests in your buildTarget (as in the hercules-ci-agent example).

If you don't list them in your buildTarget then the checkPhase will fail with "Error: Setup: No test suites enabled." (before these changes, it would fail with "cannot execute: required file not found").

wolfgangwalther · 2025-03-30T19:38:02Z

pkgs/development/haskell-modules/generic-builder.nix

This comment deserves to be in-code, I guess.

Do we really need to loop and mv each file separately, though?

We could also just do something like:

# Glob expansion can fail, when $packageConfFile is an empty directory. mv "$packageConfFile/"* "$packageConfDir" || true

Although, I am missing why the directory can be empty? Is this a separate bug fix?

wolfgangwalther · 2025-03-30T19:38:51Z

pkgs/development/haskell-modules/generic-builder.nix

Yeah, we don't need to merge the last commit, we are rebuilding everything anyway on current haskell-updates.

thomie

@wolfgangwalther Thank you for your review.

thomie · 2025-03-31T07:15:14Z

pkgs/development/haskell-modules/generic-builder.nix

Hm, silently disabling the tests feels odd to me.

Note that this change does not disable the tests, it merely disables configuring the tests with --enable-tests when a buildTarget is set.

How about changing the default values for doCheck and doBenchmark to be false when a buildTarget is set, instead? Then, they could still be overwritten.

No I don't think that works.

doCheck currently does 2 things:

configure the haskell package with --enable-tests

tell nix to run the checkPhase

Say a package sets some buildTarget (including a test-suite), and explicitly sets doCheck=True.

With your suggestion, we would still pass --enable-tests together with the buildTarget to Setup --configure, and Cabal would complain about incompatible options.

On the other hand, if the package sets the same buildTarget (including a test-suite) but doesn't override doCheck, then the test-suite would never run.

(It is possible to specify the build target in a way that would include the tests, right?)

Yes, see for example here:

nixpkgs/pkgs/development/tools/continuous-integration/hci/default.nix

Line 26 in 49ca8bc

buildTarget = "lib:hercules-ci-agent hercules-ci-agent-unit-tests";

thomie · 2025-03-31T07:47:12Z

pkgs/development/haskell-modules/generic-builder.nix

mv "$packageConfFile/"* "$packageConfDir" || true

Hm, that would silence other errors that might occur during the mv.

How about instead of this for loop, we test whether the directory is non-empty:

if [ -d "$packageConfFile" ] && [ "$(ls -A "$packageConfFile")" ];

Although, I am missing why the directory can be empty? Is this a separate bug fix?

The directory can now be empty when buildTarget doesn't include any libraries (or targets that depend on libraries), but the package does contain at least one library (so isLibrary=true). It's not a separate bug fix. Before, the register step would already have failed in this scenario.

One could argue there is another bug in this file, and that is the existence or usage of the isLibrary variable. It's not quite clear what isLibrary means or is supposed to do when a buildTarget is set. Perhaps we should try to remove the isLibrary variable, and make sure all code works whether we're configuring/building/installing a library or not. For example, after this change set, I think we can already remove the if clause from the start of the installPhase, but I didn't try it:

${if !isLibrary && buildTarget == "" then "${setupCommand} install" # ^^ if the project is not a library, and no build target is specified, we can just use "install". else if !isLibrary then "${setupCommand} copy ${buildTarget}" # ^^ if the project is not a library, and we have a build target, then use "copy" to install # just the target specified; "install" will error here, since not all targets have been built.

Show that not building the test-suite (that is mentioned in the .cabal file) is a failure when doCheck=true.

Adding a seemingly harmless and unused library component to the test for setBuildTarget makes the test fail in the installPhase (`./Setup register ..`) with: Error: Setup: '/nix/store/ah2iqzixws3fkhvlml0859hqzjjrvgsz-ghc-9.8.4/bin/ghc' exited with an error: <command line>: Could not find module ‘SomeModule’. Use -v to see a list of the files searched for.

When a package specifies a buildTarget, we now configure only that buildTarget instead of all targets. == Justification == Currently, the generic builder essentially runs: ./Setup configure ./Setup build ${buildTarget} ./Setup copy ./Setup register Now consider the scenario of haskell package containing a library and an executable (foo), but the executable does not depend on the library. In case the nix package defines `buildTarget=foo`, we do configure the library (since we always configure all targets), but we don't build it (we only build the foo executable). This causes a problem in the register step, because Setup will try to register a library that hasn't been built. To fix, we change the generic builder to run: ./Setup configure ${buildTarget} ./Setup build ${buildTarget} ./Setup copy ./Setup register Should save some cpu cycles as well. Relevant: NixOS#120249. Fixes `pkgs.tests.haskell.setBuildTarget.pass-setBuildTarget`

wolfgangwalther · 2025-04-12T17:24:33Z

(not actionable for this PR, just thinking out loud)

When looking at this, I wonder whether we should teach cabal2nix to represent the different build targets in a cabal file separately, including their respective dependencies. And then we could have the generic builder build each separately. Hm... now that I write this down, this seems like a generalization of #305958.

TeofilC · 2025-05-23T10:28:37Z

(not actionable for this PR, just thinking out loud)

When looking at this, I wonder whether we should teach cabal2nix to represent the different build targets in a cabal file separately, including their respective dependencies. And then we could have the generic builder build each separately.

This is something I would also be keen on. I created an issue for it here: NixOS/cabal2nix#603. Someone else also has some similar work here: NixOS/cabal2nix#624.

A key issue with something like this is that it would cause an explosion in the size of the generated nix code, so it would have to be an optional thing that isn't enabled in nixpkgs (if I understand the maintainer's viewpoint correctly).

github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Mar 22, 2025

nix-owners bot requested review from maralorn and sternenseemann March 22, 2025 23:38

thomie commented Mar 22, 2025

View reviewed changes

NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Mar 22, 2025

thomie marked this pull request as draft March 23, 2025 00:47

thomie force-pushed the configure-build-target branch from 78905bc to 51c9843 Compare March 23, 2025 10:58

github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 23, 2025

thomie commented Mar 23, 2025

View reviewed changes

thomie marked this pull request as ready for review March 24, 2025 00:48

nix-owners bot requested a review from roberth March 24, 2025 00:56

wolfgangwalther reviewed Mar 30, 2025

View reviewed changes

roberth removed their request for review March 30, 2025 21:58

thomie commented Mar 31, 2025

View reviewed changes

thomie force-pushed the configure-build-target branch from 51c9843 to cd12ed2 Compare April 6, 2025 20:12

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 6, 2025

thomie added 3 commits April 7, 2025 11:15

tests.haskell.setBuildTarget: test a build failure

dc6d981

Show that not building the test-suite (that is mentioned in the .cabal file) is a failure when doCheck=true.

thomie force-pushed the configure-build-target branch from cd12ed2 to 9abc2f3 Compare April 7, 2025 09:32

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 7, 2025

sternenseemann mentioned this pull request Apr 22, 2025

haskellPackages: Stackage LTS 22 -> 23; ghc: 9.6.6 -> 9.8.4 #371032

Merged

nixpkgs-ci bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 12.first-time contribution This PR is the author's first one; please be gentle! labels Jun 25, 2025

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 19, 2025

Uh oh!

Conversation

thomie commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposal

Justification

Commits:

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther commented Apr 12, 2025

Uh oh!

TeofilC commented May 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomie commented Mar 22, 2025 •

edited

Loading