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

Add docstrings to Derivation/SimpleDerivation WIP #1827

Closed
wants to merge 1 commit into from

Conversation

Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Jan 31, 2018

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

@grahamc
Copy link
Member

grahamc commented Jan 31, 2018

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;
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is correct.

@Profpatsch
Copy link
Member Author

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"` */
Copy link
Member

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

Copy link
Member Author

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?

@grahamc
Copy link
Member

grahamc commented Jan 31, 2018

Just a few comments, overall looks good to me.

@edolstra
Copy link
Member

I don't really see a need to add docstrings to the 1.11 branch.

@Profpatsch
Copy link
Member Author

What about the contents of the docstrings? Are they correct? What about the TODO?

@edolstra
Copy link
Member

The semantics of store derivations are described in https://nixos.org/~eelco/pubs/phd-thesis.pdf, especially page 40 and 101.

@Profpatsch
Copy link
Member Author

Profpatsch commented Feb 2, 2018

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. That’s also the reason I’d like to add docstrings after I understand the types.

@grahamc
Copy link
Member

grahamc commented Feb 2, 2018 via email

@copumpkin
Copy link
Member

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.

@Profpatsch
Copy link
Member Author

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.

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.

@shlevy shlevy added the backlog label Apr 1, 2018
@shlevy shlevy self-assigned this Apr 1, 2018
@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 13, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants