Skip to content

Comments

tests.haskell: prevent unnecessary rebuilds#320572

Merged
maralorn merged 2 commits intoNixOS:masterfrom
philiptaron:issue-301014/pkgs-test-haskell
Jun 19, 2024
Merged

tests.haskell: prevent unnecessary rebuilds#320572
maralorn merged 2 commits intoNixOS:masterfrom
philiptaron:issue-301014/pkgs-test-haskell

Conversation

@philiptaron
Copy link
Contributor

Description of changes

Prevent unnecessary rebuilds when Nix code is reformatted.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
    • nix-build -A tests.haskell.setBuildTarget
    • nix-build -A tests.haskell.cabalSdist.localFromCabalSdist
  • Fits CONTRIBUTING.md.

@philiptaron philiptaron requested a review from infinisil June 17, 2024 16:50
@github-actions github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Jun 17, 2024
@ofborg ofborg 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 Jun 17, 2024
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 seems reasonable. Thanks

@maralorn
Copy link
Member

Hugh, I didn’t know this was a thing. Cool.

@maralorn maralorn merged commit bfcf827 into NixOS:master Jun 19, 2024
@philiptaron philiptaron deleted the issue-301014/pkgs-test-haskell branch June 19, 2024 16:23
@sternenseemann
Copy link
Member

sternenseemann commented Jun 20, 2024

This change breaks the resulting tests, namely the assumption tests. Revert at #321246.

Serves to illustrate that a) all changes should be tested properly even if it appears that making them couldn't possibly break anything (I admit, in this case it is a little surprising, but ofborg correctly reports the other tests as changed!) and b) grepping for src = ./. is not enough, as the ./local path would also contain generated.nix. This is maybe a useful hint for work on #301014 that all local paths in nixpkgs will eventually need to be checked.

@maralorn
Copy link
Member

maralorn commented Jun 20, 2024

Sorry. but serious question: What's the correct workflow to catch this. I remember verifying that ofBorg built the tests correctly. Clearly that was not enough, as I didn't know there were more tests affected. My general stance was, that we will see build errors once they reach hydra, (which is a luxury we would have if we merge into haskell-updates, I should insist on that more)...

@philiptaron
Copy link
Contributor Author

philiptaron commented Jun 21, 2024

Yeah, I'd love to learn. I thought I did my due diligence (see the tests listed).

I see after investigation into the breadcrumbs you left in #321246 that the test is:

  • tests.haskell.cabalSdist.assumptionLocalHasDirectReference

This test (along with others) were added in #174176 by @roberth.

Specifically, what I'd like to learn is:

  1. What is tests.haskell.cabalSdist.assumptionLocalHasDirectReference actually testing? The builtins.unsafeDiscardOutputDependency appears to be load bearing there, and there's no documentation of that function that I can find. What's going on there?!
  2. Why adding the default.nix file back in caused the test to pass. How is the presence or absence of the file causing Nix to have different behavior here?

@roberth
Copy link
Member

roberth commented Jun 26, 2024

  1. What is tests.haskell.cabalSdist.assumptionLocalHasDirectReference actually testing?

A non-essential property of the way Haskell derivations are put together. If the assumption is broken, that suggests that the test may need to be updated, specifically because the more relevant test localHasNoDirectReference may succeed even if buildFromCabalSdist becomes a no-op by mistake.

  1. The builtins.unsafeDiscardOutputDependency appears to be load bearing there, and there's no documentation of that function that I can find. What's going on there?!

It turns off the implicit behavior that Nix will build the outputs of a .drv if you refer to it like that. It's a performance optimization, and not really load bearing. It just makes it so that you're not substituting many unnecessary gigabytes.
Can't find the right phrasing right now, so an issue it is

2. Why adding the default.nix file back in caused the test to pass.

The test just treats ${./local} as a constant. In any test, if you change a single constant literal to a different value, you can expect it to fail. Take for example assert 1 == 1;; change one of the integer literals, and then it fails.
This is so obvious that I guess you might feel offended. Of course it's not obvious in this case, because the literals are in different files, and they don't even look the same, so how could you know.

  • ./. in generated.nix
  • ./local in the test

Both refer to the same thing. I just didn't communicate my intent.

@roberth roberth mentioned this pull request Jun 26, 2024
13 tasks
@roberth
Copy link
Member

roberth commented Jun 26, 2024

I've opened #322556, in case it helps.

@philiptaron
Copy link
Contributor Author

That explains it. Thanks, Robert.

infinisil added a commit to tweag/nixpkgs that referenced this pull request Aug 25, 2024
The generated file sets its own directory as the source, including the
generated file itself, which causes rebuilds when that file is
reformatted. We can avoid this by overriding the source with a filtered
version and using that throughout the tests.

See NixOS#320572 for more context
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 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants