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

Context introspection #2628

Merged
merged 3 commits into from Feb 12, 2019
Merged

Context introspection #2628

merged 3 commits into from Feb 12, 2019

Conversation

@shlevy
Copy link
Member

@shlevy shlevy commented Jan 14, 2019

Add primops to inspect and set context arbitrarily from the Nix language.

Previously, plain derivation paths in the string context (e.g. those
that arose from builtins.storePath on a drv file, not those that arose
from accessing .drvPath of a derivation) were treated somewhat like
derivaiton paths derived from .drvPath, except their dependencies
weren't recursively added to the input set. With this change, such
plain derivation paths are simply treated as paths and added to the
source inputs set accordingly, simplifying context handling code and
removing the inconsistency. If drvPath-like behavior is desired, the
.drv file can be imported and then .drvPath can be accessed.

This is a backwards-incompatibility, but storePath is never used on
drv files within nixpkgs and almost never used elsewhere.
@shlevy shlevy requested a review from edolstra Jan 14, 2019
@shlevy
Copy link
Member Author

@shlevy shlevy commented Jan 14, 2019

On top of #2626 for convenience, can rebase if desired.

@edolstra
Copy link
Member

@edolstra edolstra commented Jan 14, 2019

getContext and appendContext are not actually unsafe, are they?

BTW could you put these in a separate file (e.g. primops/context.cc)? I'd like to move away from having a giant primops.cc.

src/libexpr/primops.cc Outdated Show resolved Hide resolved
This can be very helpful when debugging, as well as enabling complex
black magic like surgically removing a single dependency from a
string's context.
@shlevy shlevy force-pushed the context-introspection branch from e4acec6 to 948c879 Jan 14, 2019
@shlevy
Copy link
Member Author

@shlevy shlevy commented Jan 14, 2019

getContext and appendContext are not actually unsafe, are they?

I guess without one of the context discarding functions they're not, though I do think there's value in most of the time considering string context opaque and quasi-unobservable. Changed the names anyway.

BTW could you put these in a separate file (e.g. primops/context.cc)? I'd like to move away from having a giant primops.cc.

Done

@shlevy
Copy link
Member Author

@shlevy shlevy commented Jan 21, 2019

@edolstra Any concerns?

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 23, 2019

Yes it would be really nice to merge this soon. We use this for some testing.

@edolstra
Copy link
Member

@edolstra edolstra commented Jan 31, 2019

Running make installcheck I get:

    error: Context key '"/run/user/1000/nix-test/store/drr5fflabsdgyv90sbgc6mrpzswzgh15-eval-okay-context-introspection.nix"' is not a valid store path, at 0x1a5d74d0
    FAIL: eval-okay-context-introspection should evaluate

@shlevy
Copy link
Member Author

@shlevy shlevy commented Jan 31, 2019

Ah I guess this is because isValidPath doesn't know about paths "added" during the current eval in read-only mode? Is there an alternative?

A partner of builtins.getContext, useful for the same reasons.
@shlevy shlevy force-pushed the context-introspection branch from 948c879 to b30be6b Jan 31, 2019
@shlevy
Copy link
Member Author

@shlevy shlevy commented Jan 31, 2019

@edolstra Fixed, followed the logic in builtins.storePath

@shlevy
Copy link
Member Author

@shlevy shlevy commented Feb 12, 2019

Ping?

outputs: If a non-empty list, the relevant path is a derivation
and the provided outputs are referenced in the context
(i.e. the kind of context you get when referencing
.outPath of some derivation). Empty list if missing.
Copy link
Member

@edolstra edolstra Feb 12, 2019

Choose a reason for hiding this comment

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

I have trouble interpreting "False / empty list if missing". This paragraph should presumably read "If the path is a derivation, then this attribute is a list ... Otherwise this attribute is omitted."

However, I wonder if it might be easier for the caller to always include this attributes with values false and []?

Copy link
Member

@edolstra edolstra Feb 12, 2019

Choose a reason for hiding this comment

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

I guess it makes sense with respect to appendContext to not include them.

@edolstra edolstra merged commit 7a7ec22 into NixOS:master Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants