From 20ab913c390105728162bbff981a421f657212c4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 30 Oct 2019 21:39:08 -0400 Subject: [PATCH 1/5] ci-all-nix-prs: Copy Template --- rfcs/0000-ci-all-nix-prs.md | 49 +++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 rfcs/0000-ci-all-nix-prs.md diff --git a/rfcs/0000-ci-all-nix-prs.md b/rfcs/0000-ci-all-nix-prs.md new file mode 100644 index 000000000..e3d4c0864 --- /dev/null +++ b/rfcs/0000-ci-all-nix-prs.md @@ -0,0 +1,49 @@ +--- +feature: (fill me in with a unique ident, my_awesome_feature) +start-date: (fill me in with today's date, YYYY-MM-DD) +author: (name of the main author) +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 + +One paragraph explanation of the feature. + +# Motivation +[motivation]: #motivation + +Why are we doing this? What use cases does it support? What is the expected +outcome? + +# Detailed design +[design]: #detailed-design + +This is the bulk of the RFC. Explain the design in enough detail for somebody +familiar with the ecosystem to understand, and implement. This should get +into specifics and corner-cases, and include examples of how the feature is +used. + +# Drawbacks +[drawbacks]: #drawbacks + +Why should we *not* do this? + +# Alternatives +[alternatives]: #alternatives + +What other designs have been considered? What is the impact of not doing this? + +# Unresolved questions +[unresolved]: #unresolved-questions + +What parts of the design are still TBD or unknowns? + +# Future work +[future]: #future-work + +What future work, if any, would be implied or impacted by this feature +without being directly part of the work? From da00dabd58f8de9ae4b6f0b1172c03667a305129 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 30 Oct 2019 21:51:23 -0400 Subject: [PATCH 2/5] ci-all-nix-prs: Draft --- rfcs/0000-ci-all-nix-prs.md | 41 +++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/rfcs/0000-ci-all-nix-prs.md b/rfcs/0000-ci-all-nix-prs.md index e3d4c0864..1d399b9f7 100644 --- a/rfcs/0000-ci-all-nix-prs.md +++ b/rfcs/0000-ci-all-nix-prs.md @@ -1,7 +1,7 @@ --- -feature: (fill me in with a unique ident, my_awesome_feature) -start-date: (fill me in with today's date, YYYY-MM-DD) -author: (name of the main author) +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) @@ -11,39 +11,50 @@ related-issues: (will contain links to implementation PRs) # Summary [summary]: #summary -One paragraph explanation of the feature. +Build all Nix PRs in CI. +Do not merge any PR until it passes CI. # Motivation [motivation]: #motivation -Why are we doing this? What use cases does it support? What is the expected -outcome? +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. + +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 -This is the bulk of the RFC. Explain the design in enough detail for somebody -familiar with the ecosystem to understand, and implement. This should get -into specifics and corner-cases, and include examples of how the feature is -used. +Set up Hydra declarative jobsets to build all Nix PRs. +Those with merge access should be instructed not to merge a PR until CI passes. +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. + +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 -Why should we *not* do this? +More process to follow. # Alternatives [alternatives]: #alternatives -What other designs have been considered? What is the impact of not doing this? +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 -What parts of the design are still TBD or unknowns? +Nothing. # Future work [future]: #future-work -What future work, if any, would be implied or impacted by this feature -without being directly part of the 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 From 317f7ee84c0c2245f3ce11842431cbf252d5ef00 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 11 Nov 2019 22:32:51 -0500 Subject: [PATCH 3/5] ci-all-nix-prs: Clean up - Only build approved PRs - Mention OfBorg - Mention fixed output security risk - Word-smith - Expand some misc text - Fix link --- rfcs/0000-ci-all-nix-prs.md | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/rfcs/0000-ci-all-nix-prs.md b/rfcs/0000-ci-all-nix-prs.md index 1d399b9f7..c89aad6f5 100644 --- a/rfcs/0000-ci-all-nix-prs.md +++ b/rfcs/0000-ci-all-nix-prs.md @@ -17,7 +17,7 @@ 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. +There is a [famous blog post](blog-post) about how 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. @@ -27,7 +27,10 @@ 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. +Optional first step: we can set up OfBorg to build all PRs. + +Set up Hydra declarative jobsets to build all approved Nix PRs. +This might involve extending Hydra somewhat. Those with merge access should be instructed not to merge a PR until CI passes. 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. @@ -43,7 +46,22 @@ 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. +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. + 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 @@ -53,8 +71,10 @@ Nothing. # Future work [future]: #future-work -- Remove the manual parts of this process. +- 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 to other smaller official repos, like `cabal2nix`. +- Also apply process to other smaller official repos, like `cabal2nix`. [blog-post]: https://graydon2.dreamwidth.org/1597.html From cca84afda7757393ad2062503e6f0ee095a805d1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 17 Nov 2019 17:34:12 -0500 Subject: [PATCH 4/5] ci-all-nix-prs: Fix markdown error Thanks! Co-Authored-By: asymmetric --- rfcs/0000-ci-all-nix-prs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0000-ci-all-nix-prs.md b/rfcs/0000-ci-all-nix-prs.md index c89aad6f5..1627202fc 100644 --- a/rfcs/0000-ci-all-nix-prs.md +++ b/rfcs/0000-ci-all-nix-prs.md @@ -17,7 +17,7 @@ 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. +There is a [famous blog post][blog-post] about how 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. From 6fe117ee3649c031884be620b014a5bba50c99dd Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 17 Nov 2019 17:36:15 -0500 Subject: [PATCH 5/5] ci-all-nix-prs: Clarify ambiguity Thanks! Co-Authored-By: asymmetric --- rfcs/0000-ci-all-nix-prs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0000-ci-all-nix-prs.md b/rfcs/0000-ci-all-nix-prs.md index 1627202fc..1b8a1a056 100644 --- a/rfcs/0000-ci-all-nix-prs.md +++ b/rfcs/0000-ci-all-nix-prs.md @@ -49,7 +49,7 @@ More process to follow. 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. +2. Merely build all approved PRs with Hydra, but maintainers are still allowed to merge broken ones / not take care to avoid untested merge commits. 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.