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.

Sign up for GitHub

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

Jump to bottom

[RFC 0095] Enable doCheck by default #95

Closed
wants to merge 11 commits into from
Closed

[RFC 0095] Enable doCheck by default #95

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cc112ac
[RFC 0095] Enable doCheck by default
gytis-ivaskevicius Jun 25, 2021
8c47ee8
[RFC 0095] Pin commit so that the lines don't move around
gytis-ivaskevicius Jun 26, 2021
ad5adb6
[RFC 0095] Remove unecessary space
gytis-ivaskevicius Jun 26, 2021
864572c
[RFC 0095] 'if' -> 'If'
gytis-ivaskevicius Jun 26, 2021
af3d757
[RFC 0095] Add code blocks around values
gytis-ivaskevicius Jun 26, 2021
b8924de
[RFC 0095] Add code block around 'checkPhase'
gytis-ivaskevicius Jun 26, 2021
918edff
[RFC 0095] update 'doCheck'/'doInstallCheck' semantics
gytis-ivaskevicius Jun 30, 2021
5980692
Update rfcs/0095-enable-docheck-by-default.md
edolstra Sep 2, 2021
680344d
Update rfcs/0095-enable-docheck-by-default.md
gytis-ivaskevicius Sep 14, 2021
4b75420
Update drawbacks
gytis-ivaskevicius Nov 15, 2021
a39020b
Update alternatives
gytis-ivaskevicius Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions rfcs/0095-enable-docheck-by-default.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
---
feature: enable-docheck-by-default
start-date: 2021-06-24
author: gytis-ivaskevicius
co-authors:
shepherd-team: @Ericson2314 @nh2 @edolstra
shepherd-leader: @Ericson2314
related-issues:
---

# Summary
[summary]: #summary

Enable `doCheck` by default when using `stdenv.mkDerivation` function.

# Motivation
[motivation]: #motivation

I believe that an additional quality gate would be beneficial to the derivations build process.

Resolution of this RFC is expected to remove [these comments](https://github.com/NixOS/nixpkgs/blob/8c563eaf7049d82fbe95b0847ac5ae6e5554e2fa/pkgs/stdenv/generic/make-derivation.nix#L61-L67)
either by enabling `checkPhase` by default or rejecting this RFC.

# Detailed design
[design]: #detailed-design

The basic idea is quite simple
- New `doCheck`/`doInstallCheck` semantic should be implemented.
- By default `doCheck` option should be enabled as long as `stdenv.hostPlatform == stdenv.buildPlatform`.
- Non-reproducible test prevention should be implemented.
- All failing packages should be fixed or updated with `doCheck = false;`

Copy link
Author

@gytis-ivaskevicius gytis-ivaskevicius Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
Guidelines to refrain from enabling tests:
- If tests are taking _too_ long. (Tests aren't expected to run longer than build time. In case of quick builds - not more than 10min)
- If tests require additional large dependencies.
- If tests are flaky. (If tests randomly fail once in a while)

Copy link
Author

Choose a reason for hiding this comment

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

Guys, sounds good?

Copy link
Member

Choose a reason for hiding this comment

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

Guidelines to refrain from enabling tests

Copy link
Member

Choose a reason for hiding this comment

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

P.S.:

  • s|If|When|
  • When tests are flaky, unpredictably failing.

**New `doCheck`/`doInstallCheck` semantics:**
In addition to booleans, `doCheck`/`doInstallCheck` should also accept strings.
- String value should be considered as `false`
- It should be used as a place for comment on why the check is disabled. For
example: "Requires X11 server" or "Requires network access".
Comment on lines +33 to +37
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
**New `doCheck`/`doInstallCheck` semantics:**
In addition to booleans, `doCheck`/`doInstallCheck` should also accept strings.
- String value should be considered as `false`
- It should be used as a place for comment on why the check is disabled. For
example: "Requires X11 server" or "Requires network access".
**New semantics:**
- `doCheck`/`doInstallCheck` should default to `null` and work exactly the same as `false`
- New options should be introduced: `meta.{checksFlaky,checksLargeDependencies,checksTakeTooLong,checksDisableReason}`

The first point is so we could evaluate checks that are not set. Derivation paths are not expected to change.
And the second point is so we could evaluate the reason why checks were disabled. Maybe defining additional attrset would be nice? like checks.{flaky,largeDependencies,takeTooLong,disableReason}? Any thoughts?

Copy link
Member

@AndersonTorres AndersonTorres Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A comment on the source code and a Boolean value shoud suffice. There is no reason to overload this.

Copy link
Author

Choose a reason for hiding this comment

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

During RFC call, we thought that it would be a very simple change which would allow us to actually evaluate the reason why tests were disabled and that is definitely handy. I am still in favor of it

Copy link
Member

Choose a reason for hiding this comment

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

That being said, it is preferrable a (short?) string, without true/false semantics attached to it.
After all, it can be useful to use the string to convey a useful information even in the case the tests are mandatory.

Something like

doCheck = false;
checkReason = "I don't wanna fetch X.Org just to verify a blinking box!";

Or even better, a new attrset:

check.enable = true;
check.reason = "This package is critical for bootstrap";


**Non-reproducible tests prevention:**
There are multiple options. Here I am going to list a few:
1. `chmod a-w -R /build`
2. [User namespaces](https://lwn.net/Articles/532593/)
3. Generate unique identifier from existing sources and compare it with
   identifier generated after executing `checkPhase`. `exit 1` if values
   mismatch. (Identifier can be generated by something simple like `du -s`)
gytis-ivaskevicius marked this conversation as resolved.
Show resolved Hide resolved
4. [unionfs](https://en.wikipedia.org/wiki/UnionFS)

# Drawbacks
[drawbacks]: #drawbacks

- Increased build time.
- More non-deterministic build failures.
- Extra dependencies for the test framework.
- Upstream tests don't often reveal downstream packaging/integration issues, because most are functional tests that are unlikely to break.

# Alternatives
[alternatives]: #alternatives

If enabling `doCheck` globally is too expensive, there are some ideas for running tests anyway:
- Let ofborg build pkg.overrideAttrs { doCheck = true; }. That way our CI runs tests but users who build from source don't have to.
- Have more .passthru.test derivations to test installed packages.
- Split tests into separate derivations, e.g. by saving the build tree into a separate output and running the test from there. This would be quite expensive for Hydra in terms of storage space, since build trees are large.

# Unresolved questions
[unresolved]: #unresolved-questions

"Non-reproducible tests prevention" implementation is to be decided. I feel
like `du -s` is the right way to go about it since it is simple/fast and I
expect it to be quite reliable.