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

tree-wide: disable doCheck and doInstallCheck where it fails #39463

Merged
merged 1 commit into from Apr 25, 2018

Conversation

oxij
Copy link
Member

@oxij oxij commented Apr 25, 2018

(the trivial part)

Motivation for this change

#33599. I implemented that issue for fun and giggles and now run a system that is "fully-tested". Turns out, not unexpectedly, an enormous number of packages fail to check. This is the first patch from that patchset. This patch disables many things that fail, but I couldn't (even partially) fix them. I.e. this patch contains only trivial doCheck = false changes and a couple of cleanups. The rest is coming later.

Note that I started documenting error messages too late, hence many changes here just disable tests without comments. I'm too lazy to repeat the effort to document the "doCheck = false; # fails" lines as rebuilding and fixing everything multiple times with 33599-patchset took almost two weeks.

Note: For most packages you won't be able to check why they actually fail with just doCheck = true; without some more patches from the patchset because the patchset adds checkTraget autodetection to stdenv.

Things done
  • When combined with other patches in the series the system builds.
  • Fits CONTRIBUTING.md.

@oxij
Copy link
Member Author

oxij commented Apr 25, 2018

For checkTarget autodetection see #39464.

@7c6f434c
Copy link
Member

I think might be interested: @grahamc @Profpatsch @Ekleog

Have tried to check that all the cleanups are pure layout changes (toward more uniformity). I think I have read every change; the cleanups are pure cleanups.

Please remind me in a few days where few is more than a couple, if nobody merges it before.

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

This PR adds doCheck = false to GHC version 8.2.2, but not to any of the other versions we have even though they'll surely need the same setting. I suppose this PR cannot be expected to be "complete" since that would be a lot of packages to compile, but still it might be worthwhile to point out that switching to doCheck = true would break a lot of packages -- even after these changes.

@Profpatsch
Copy link
Member

Ugh, this leads to rebuilds because mkDerivation is still not declarative enough. In a perfect world this patch would be a no-op. :(

@7c6f434c
Copy link
Member

7c6f434c commented Apr 25, 2018 via email

@oxij
Copy link
Member Author

oxij commented Apr 25, 2018

@peti

This PR adds doCheck = false to GHC version 8.2.2, but not to any of the other versions we have even though they'll surely need the same setting.

Interestingly, no, at least not for the older ones. I compiled a bunch of them and only this one was broken for some reason.

I suppose this PR cannot be expected to be "complete"

This PR is maybe a third of all the changes. And even then I didn't try to recompile every single thing, so there are going to be errors even after (and if) the whole patchset is merged.

Also I wouldn't recommend switch doCheck to true for Hydra by default (unless we make that into an impurity), but ofborg could totally do selective doCheck by default.

@7c6f434c

This PR would be a rebuild anyway: there are some cleanups in the layout of the shell script parts of some expressions.

In the perfect world mkDerivation would normalize shell scripts that that would be a noop too :)

@7c6f434c
Copy link
Member

7c6f434c commented Apr 25, 2018 via email

@7c6f434c
Copy link
Member

@peti do I understand your comment as «not complete but an OK step in the general direction»? Looking at the feedback I am inclined to just merge it before we get a formatting-based merge conflict or something like that.

@peti
Copy link
Member

peti commented Apr 25, 2018

@7c6f434c, yes, sure. I am happy with the direction of this PR.

@7c6f434c 7c6f434c merged commit 9587c6c into NixOS:staging Apr 25, 2018
@oxij
Copy link
Member Author

oxij commented Apr 25, 2018

Nice. I updated #39464 with most of the infrastructure. See the OP there.

@oxij oxij mentioned this pull request Apr 28, 2018
8 tasks
@oxij oxij mentioned this pull request May 29, 2018
1 task
@oxij oxij deleted the stdenv/docheckfalse-trivial branch September 8, 2018 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants