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 0014] Improve import from derivation #14

Closed
wants to merge 10 commits into from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jun 3, 2017

New description

Build on #40 to allow pausing and resuming evaluation with derivations, such that we always have derivations for are end goals. This allows hydra and various nix-env queries to work with their current architecture. Therefore, if this RFC is implemented, hydra.nixos.org should allow importing from derivations.


Original description

Fix some issues with importing from derivations so it is nicer to use both manually and with CI. If this RFC is implemented, hydra.nixos.org should allow importing from derivations.


Rendered

Copy link
Member

@shlevy shlevy left a comment

Choose a reason for hiding this comment

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

🙌

Happy to help out in any way with this!

@zimbatm
Copy link
Member

zimbatm commented Jun 6, 2017

@Ericson2314 did you get my review by email? Let me know what I can do to help.

There's currently a bug with `nix-repl` where if computation is interrupted (say with `SIGINT`), and the same computation is begun again, one will get an infinite recursion error despite there being no infinite recursion.
This is because the thunks stay black-holed—the indication that they are currently evaluated—despite the computation being aborted.
To fix this, we want aborted computations to instead unwind their stack, un-black-holing any thunks back the way they were before.
Mathematically, black-hole unwinding ensures that evaluation is idempotent.
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought: this seems useful on it's own. Maybe split it in it's own RFC, I suspect smaller RFCs are more likely to make progress. Or just implement it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing that that's just a bug fix, I might just open an issue. That issue can be linked from any tracking the RFC as a whole if it is accepted.

Copy link
Member Author

Choose a reason for hiding this comment

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

# Drawbacks
[drawbacks]: #drawbacks

Overlap with recursive Nix.
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a link to what recursive Nix is? Just so the reader can compare.

@0xABAB
Copy link

0xABAB commented Jun 30, 2017

The technical writing could be a lot better, but I get the gist of it.

It appears that this is just an instance of "recursive make considered harmful", though. My longterm concern is that this will lead to a new workflow where people will start making use of the feature, which means that the system as a whole becomes dependent on more dynamic features while they could also be implemented with more static features (as it is now).

In short, I would say this is a controversial feature. It's not like I can stop you, but I probably wouldn't add this feature if I had complete control over the project.

@Ericson2314
Copy link
Member Author

Yes, dynamism should be the escape the escape hatch of last resort. I hope the burden of writing concrete syntax with an external program is enough to dissuade anyone from using it more.

@Ericson2314
Copy link
Member Author

I should edit this RFC to also mention readFile. ReadFile of a derivation needs analogous improvements.

@domenkozar
Copy link
Member

domenkozar commented Jul 19, 2017

So if I understand correctly, to summarize:

  1. Hydra would evaluate an expression until it hits IFD and halt there
  2. it would use build slaves to build the IFD paths
  3. start with 1) using the new built paths to continue the eval

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 20, 2017

@domenkozar do you mean " start 1) again..."? If so yes. It could also just let nix-instantiate do that automatically, but avoid keeping the eval process around it's probably best to do that by hand with --dry-run. [That remainds me, I should make clear that with these changes --dry-run is now meaningful for nix-instantiation too, whereas today we are stuck without it.]

@edolstra
Copy link
Member

@Ericson2314 We're doing triage on the RFC backlog, are you still interested in pursuing this RFC?

The issue I see with this RFC is that it doesn't really work given Hydra's current design, specifically, the strict split between the evaluator and the queue runner. The evaluator is supposed to add rows to the Builds table, so it can't delay building IFDs - it has to produce a top-level derivation. Likewise the queue runner doesn't have access to the jobset inputs, so it can't resume evaluation.

This could be solved by having an integrated evaluator/queue runner, but then we don't need any partial evaluation tricks: we can just build IFDs as normal build steps during evaluation. (The only issue is that during evaluation there is no Build yet to account the BuildStep to, but that can be hacked around by having a temporary Build or whatever.)

The downside of integrating the evaluator and queue runner is that it reduces security a bit, since (for example) the jobset Nix expression could try to read SSH credentials of the queue runner during evaluation using readFile.

@Ericson2314
Copy link
Member Author

I'm going to close for the time being. I need to fix a few details, and then I have a new idea using derivations that build derivation trees to do this better still.

@Ericson2314
Copy link
Member Author

@edolstra Also, thank you for that description of the hydra status quo. I'll try to better separate the Nix itself and hydra concerns from this PR, but on the hydra side I'm thinking that top-level derivations must not be auto-generated, but their dependencies may be. My new idea already supported that on the Nix side, and from your description, that would be allow Hydra to keep its existing division of labor. [If we had to refactor hydra that much, I'd declare it "totalled", i.e. it would be better to just use a rewrite that would also fix a slew of other issues.]

@edolstra
Copy link
Member

In Nix, you can't have a .drv file without knowing all its dependencies. This is independent from any evaluation issues.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 18, 2019

@edolstra yes this would be a recursive Nix thing, where drv1 would build drv2 (and drv2's transitive dependencies' drvs). drv1 would be delayed evaluation of nix expressions, which we may or may not wish to special-case purely. That special case would purely be an optimization.

piegamesde pushed a commit to piegamesde/rfcs-1 that referenced this pull request Jan 31, 2024
)

* unindent inherit semicolon, reshuffle binding/inherit sections

* fixup! Stuff
infinisil added a commit that referenced this pull request Mar 5, 2024
* 166: Nix formatting

Create 0101-nix-formatting.md

WIP

Go through a large part and agree on it

Co-Authored-By: @piegamesde

Update 0101-nix-formatting.md

Rework a lot of things

Update 0101-nix-formatting.md

Move around some sections

Reword the detailed section

Minor updates

Slight header changes again

Updates

Update 0101-nix-formatting.md

Update after today's meeting

Update 0101-nix-formatting.md

Further updates in the meeting

Update 0101-nix-formatting.md

After todays meeting

Update after meeting

Rename to probably the right number

Only use anchor links

Improvements and additions

- The sub-expression rule is now reworded and its own section with
  examples and rationale
- Line length limit is now specified as we agreed-upon in the meeting
- The operator section is rewritten to align more with the consensus

Redo and explain operator special case

Also remove the special case for non-chainable operators, barely any benefit
in Nixpkgs

* Operator chains outside bindings can also have a compact form

* Make the operator compact form specific to binders

* Fix accidentally formatted semicolons in alternatives

* Minor changes

* Light copy editing

* Fix .git-blame-ignore-revs

* Improve assert/with wording

* Be more flexible with single-line element count

* binder -> binding

* unindent inherit semicolon, reshuffle binding/inherit sections (#14)

* unindent inherit semicolon, reshuffle binding/inherit sections

* fixup! Stuff

* Give alternatives to `in` formatting

* Expand on line break preservation

* Add editorconfig

* Expand argumentation against leading commas

* Add @dasJ to the formatter team

* Add shepherd team

Co-authored-by: Linus Heckemann <git@sphalerite.org>

* Various improvements (#15)

* Various improvements

- Remove unnecessary **Description** headers
- Rename **Rationale and Alternatives** to just **Alternatives**
- Insert must/may/should more diligently
- Add some TODOs where things are unclear
- Remove numberings from examples when not needed
- Minor clarity improvements and simplifications throughout

* Apply suggestions from code review

Co-authored-by: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>

---------

Co-authored-by: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>

* Address TODOs and rework with/assert

* Minor adjustments

* Mention formatting Nix code in documentation

* Working towards finalization (#16)

- Defined absorption and absorbable terms
- Adapted the existing RFC text to make use of these definitions,
  resulting in simplications of the text in many cases.
- Updated `with` section to match the implementation
- Updated the function declaration section to match the implementation
  - Sometimes, the function body may get absorbed
  - This used to be a special case scoped to bindings only, so it got removed there
- Updated the operators section to match the implementation
  - Specify the format of non-chainable operators (somehow those got lost in the past)
- Reworked bindings section. It should now be clear and specific enough.
- Minor wording fixes

* String section

* Specify assert conditions

* More absorption for multi-line arguments

* How to update the standard format

* Fix minor typos

* Less lines for common function call patterns

* Specify comments

* Specify that the formatter should be as pure as possible

With some exceptions

* nit: fix list concatenation example (#17)

* Update rfcs/0166-nix-formatting.md

Co-authored-by: Doron Behar <doron.behar@gmail.com>

* Add good indentation examples (#18)

* Add another chainable operators example

* justify difference in semicolon placement

* Allow different parenthesized argument style

* Clarify non-vertical alignment rule

* Improved clarity of bindings rule

* Improve bindings semicolon alternatives section

---------

Co-authored-by: Silvan Mosberger <github@infinisil.com>
Co-authored-by: Silvan Mosberger <silvan.mosberger@tweag.io>
Co-authored-by: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
Co-authored-by: Yuriy Taraday <yorik.sar@gmail.com>
Co-authored-by: Linus Heckemann <git@sphalerite.org>
Co-authored-by: Janne Heß <janne@hess.ooo>
Co-authored-by: Doron Behar <doron.behar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants