-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add docstrings to Derivation/SimpleDerivation WIP #1827
Conversation
Eelco would prefer if you sent this to master first, then adapted it for 1.11 in a cherry-picked backport. |
DerivationOutputs outputs; | ||
|
||
/* Inputs that are sources. <- TODO what’s a source? */ | ||
PathSet inputSrcs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By process of elimination, if I have cp ${./foobar} $out
then ./foobar
would be moved to the Nix store, and the Nix store path would be in the srcs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct.
Yeah, will do a separate PR for master. |
PathSet inputSrcs; | ||
|
||
/* The system architecture this derivation can be built on, | ||
as returned by `config/config.guess`, e.g. `"x86_64-linux"` */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this can be overridden by the derivation itself, ie: https://github.com/NixOS/nix/blob/master/tests/config.nix#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s worded ambiguously. The strings follow the naming of the possible values returned by the config.guess
script. How could that be rephrased?
Just a few comments, overall looks good to me. |
I don't really see a need to add docstrings to the 1.11 branch. |
What about the contents of the docstrings? Are they correct? What about the TODO? |
The semantics of store derivations are described in https://nixos.org/~eelco/pubs/phd-thesis.pdf, especially page 40 and 101. |
Does Since the paper is from a few years back, I’d expect the data structures and even some semantics to have changed quite drastically, especially in nix |
Derivation files are forward / backward compatible, as far as I can tell.
This is (what I assume) allows Nix 2.0 to be deployed to all the Hydra
builders.
…On Fri, Feb 2, 2018 at 10:19 AM Profpatsch ***@***.***> wrote:
Does DerivationOutputs outputs; contain a map from output names to
DerivationOutput? That’s not mentioned in the paper.
Since the paper is from a few years back, I’d expect the data structures
and even some semantics to have changed quite drastically, especially in
nix 2.0.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1827 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAErrCdav3BCpPIjy4PuLxaAVWNR-745ks5tQyERgaJpZM4Rz5pj>
.
|
Having said all that, having the canonical documentation for a piece of code be an immutable PDF from 12 years ago is probably not the best way to encourage documentation to improve 😄 Perhaps we can have a slightly more "live" version of this information in a place that can evolve quickly (even if the underlying concepts don't change much, the clarity of their explanations can obviously evolve independently). It seems like getting something like this into 1.12 documentation, probably with a link back to the dissertation, would be good, but probably not worth making further changes to the 1.11 codebase over. Does that make sense? At least that's my understanding of @edolstra's comment above about putting this against the maintenance branch. |
Yes, I’d have pushed the docs to both versions (for people like me who want to study 1.11 source while it’s still in use on their system), but if it’s not worth the effort I’m okay with just pushing to 1.12. |
I marked this as stale due to inactivity. → More info |
I closed this issue due to inactivity. → More info |
This should make deciphering the internal representation of a derivation a lot easier.
There’s still a
TODO
and it will probably have to be adapted a bit for 1.12.cc @edolstra @aszlig