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 0056] CI all Nix PRs #56

Closed
wants to merge 5 commits into from
Closed

[RFC 0056] CI all Nix PRs #56

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20ab913
ci-all-nix-prs: Copy Template
Ericson2314 Oct 31, 2019
da00dab
ci-all-nix-prs: Draft
Ericson2314 Oct 31, 2019
317f7ee
ci-all-nix-prs: Clean up
Ericson2314 Nov 12, 2019
cca84af
ci-all-nix-prs: Fix markdown error
Ericson2314 Nov 17, 2019
6fe117e
ci-all-nix-prs: Clarify ambiguity
Ericson2314 Nov 17, 2019
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
60 changes: 60 additions & 0 deletions rfcs/0000-ci-all-nix-prs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
---
feature: ci-all-nix-prs
start-date: 2019-10-30
author: John Ericson (@Ericson2314)
co-authors: (find a buddy later to help our with the RFC)
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

Build all Nix PRs in CI.
Do not merge any PR until it passes CI.

# Motivation
[motivation]: #motivation

There is a (famous blog post)[blog-post] that everyone is sloppy and doing CI wrong.
This isn't just bad for releasing software smoothly, but also increases the burden for new contributors because it is harder to judge the correctness of PRs at a glance (is it broken? Did I break it?).
I personally find it harder to contribute, I have to worry about double checking all my work on platforms I don't have as-easy access to, like Darwin.
Copy link
Member

@zimbatm zimbatm Nov 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Another motivation is that broken tests are not always the author's fault as master often turns red. Meaning that the author now has to checkout master to see this is a new error or not. If master is red, the author now has to either:

a. find a working commit in the history. If there are any merge conflict this might not be an option.
b. try and fix master himself. This might be completely unrelated work, meaning the author has to load and understand a new context.
c. wait or pester other contributors to fix master so they can continue their work.

Basically the later an error gets caught, the more expensive it becomes to fix.

Copy link
Member

Choose a reason for hiding this comment

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

We already have something like "rocket science" CI if you treat "nixpkgs-channels" as the source code. The channels will only update if the branch passes all of its blocking tests. This accomplishes the same goal as mentioned in the post and avoids the crazy costs that come with "rocket science" CI.

Copy link

Choose a reason for hiding this comment

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

The problem with the way we use Hydra for nixpkgs-channels is that if a broken commit is ever merged to nixpkgs, everything that gets merged after the broken commit also gets blocked from making it to nixpkgs-channels until the problem is fixed.

Don't get me wrong: It's definitely much better than some "CI" systems I've used, but it's not quite as good as the first one I used, which only blocks the broken commit until it's fixed, while allowing other (passing) commits to be merged.


We cannot yet do this for all of Nixpkgs, sadly, due to resource limits.
But, there is no reason we cannot do it for Nix itself.

# Detailed design
[design]: #detailed-design

Set up Hydra declarative jobsets to build all Nix PRs.
Copy link
Member

Choose a reason for hiding this comment

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

Having used these in the past, they rarely work well. I think the Hydra model only really works for "post-merge" CI, not "pre-merge" CI. A tool like OfBorg (or something else) seems like a good solution to "pre-merge" CI.

Those with merge access should be instructed not to merge a PR until CI passes.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can actually be enforced on GitHub, right? So why not make it part of the detailed design?

Merge master into PRs or rebase before merge as a crude stop-gap to avoid master becoming an untested tree due to a merge commit.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could Hydra build the PR and also build the merge commit of the PR into master?


If Hydra gains the ability to keep master always working obviating the manual steps above beyond the PR jobs, use that ability.

If a new CI is used, ensure that is also keeps master in an always-building state.

# Drawbacks
[drawbacks]: #drawbacks

More process to follow.

# Alternatives
[alternatives]: #alternatives

Merely build all PRs, and maintainers are still allowed to merge broken ones / not take care to avoid untested merge commits.

# Unresolved questions
[unresolved]: #unresolved-questions

Nothing.

# Future work
[future]: #future-work

- Remove the manual parts of this process.

- Also apply to other smaller official repos, like `cabal2nix`.

[blog-post]: https://graydon2.dreamwidth.org/1597.html