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

[RFC 0056] CI all Nix PRs #56

Closed
wants to merge 5 commits into from
Closed
Changes from 3 commits
Commits
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
80 changes: 80 additions & 0 deletions rfcs/0000-ci-all-nix-prs.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
---
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) about how everyone is sloppy and doing CI wrong.
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
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

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

Optional first step: we can set up OfBorg to build all PRs.

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

Choose a reason for hiding this comment

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

It would be good to define what approved means here:

  • review approved or specially formatted comment?
  • how many approvals are required?
  • who can approve?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I have no preference. We just need something to solve the small security problems.

This might involve extending Hydra somewhat.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify how Hydra would have to be extended?

I can imagine for example giving it access to GitHub's API, to know whether a PR is "approved".

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be via github approval, or a github comment, whichever is easier.

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

1. Merely build all PRs with OfBorg.
This is still far better than the status quo, but has the disadvantage that master must still be rebuilt as OfBorg and Hydra do not share a cache.

2. Merely build all approved PRs, and maintainers are still allowed to merge broken ones / not take care to avoid untested merge commits.
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
This is better still, but master could still be broken, even if only working branches are merged due to the merges not being tested.

3. Build all PRs with Hydra.
This was my original proposal, which has the benefit of not requiring new Hydra features.
Unfortunately there is a slight security risk.
While we generally trust Nix sandboxing---Nixpkgs PR reviewers do not do a full security audit or anything close---fixed output derivations have no network sandboxing.
This means a mischievous PR could is free to do some work and communicate its result to the outside world, rather than have it be lost for ever.
So yes, people could mine crypto-currency or something from within their filesystem-but-not-network sandbox.
Worse, conceivably through some side channel that Linux namespaces do not guard, a rouge fixed-output derivation could try to slowly exfiltrate secrets or something.

Only building approved PRs is a crude, but probably adequate workaround.
A rogue fixed-output derivation should be much harder to hide than arbitrary malicious code, especially as the Nix code of any PR should be understandable to our reviewers, and much smaller than the C++.

# Unresolved questions
[unresolved]: #unresolved-questions

Nothing.

# Future work
[future]: #future-work

- Remove the manual parts of the "it's not rocket science" process.
This means that either one can only merge when it would be a "fast forward" merge, or CI does the merging for your.
This enforces that the tip of master is always building and cached, even as it is pushed to a new commit, race free.

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

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