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.

Sign up for GitHub

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

Jump to bottom

Regression builtins.getContext / outputs = [ "" ] #7655

Closed
roberth opened this issue Jan 21, 2023 · 4 comments · Fixed by #7657
Closed

Regression builtins.getContext / outputs = [ "" ] #7655

roberth opened this issue Jan 21, 2023 · 4 comments · Fixed by #7657
Assignees
@Ericson2314
Labels
bug regression Something doesn't work anymore

Comments

@roberth
Copy link
Member

roberth commented Jan 21, 2023

Describe the bug

builtins.getContext always returns { outputs = [ "" ]; } for build outputs.

Steps To Reproduce

cd nixpkgs; nix repl .

Expected:

Actual:

nix-repl> :p builtins.getContext (writeText "hi" "hi").outPath
{ "/nix/store/axry75xwlchnnrqp263d05yhr89cv3d0-hi.drv" = { outputs = [ "" ]; }; }

nix-repl> :p builtins.getContext nix.doc.outPath
{ "/nix/store/56n2kf5pha0m8cs19vgzhv5j0d7mr96n-nix-2.12.0.drv" = { outputs = [ "" ]; }; }

Expected behavior

nix-repl> :p builtins.getContext (writeText "hi" "hi").outPath
{ "/nix/store/axry75xwlchnnrqp263d05yhr89cv3d0-hi.drv" = { outputs = [ "out" ]; }; }

nix-repl> :p builtins.getContext nix.doc.outPath                    
{ "/nix/store/56n2kf5pha0m8cs19vgzhv5j0d7mr96n-nix-2.12.0.drv" = { outputs = [ "doc" ]; }; }

nix-env --version output

master, 2.13.1

Additional context

Priorities

Add 👍 to issues you find important.
@roberth roberth added bug regression Something doesn't work anymore labels Jan 21, 2023
@roberth roberth mentioned this issue Jan 21, 2023
Merged
@Ericson2314 Ericson2314 self-assigned this Jan 22, 2023
@Ericson2314
Copy link
Member

Have fix, just writing tests.

@Ericson2314
Copy link
Member

tests/lang/eval-okay-context-introspection.nix is already a test, I am confused why it is passing.

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 22, 2023 •
edited
Loading

Oh nevermind, I see now.

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jan 22, 2023
@Ericson2314
Expand tests to reproduce NixOS#7655
88d8f6a 
The original `builtins.getContext` test from
1d75729 would have caught this. The
problem is that b30be6b adding
`builtins.appendContext` modified that test to make it test too much at
once, rather than adding a separate test.

We now have isolated tests for both functions, and also a property test
showing everything put together (in the form of an eta rule for strings
with context). This is better coverage and properly reproduces the bug.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jan 22, 2023
@Ericson2314
Fix NixOS#7655
0afdf40 
We had some local variables left over from the older (more
complicated) implementation of this function. They should all be unused,
but one wasn't by mistake.

Delete them all, and replace the one that was still in use as intended.
@Ericson2314 Ericson2314 mentioned this issue Jan 23, 2023
Merged
7 tasks
roberth added a commit that referenced this issue Jan 23, 2023
@roberth
Merge pull request #7657 from obsidiansystems/fix-7655
0a9acef 
Fix #7655
github-actions bot pushed a commit that referenced this issue Jan 23, 2023
@Ericson2314 @github-actions
Expand tests to reproduce #7655
3f58d5a 
The original `builtins.getContext` test from
1d75729 would have caught this. The
problem is that b30be6b adding
`builtins.appendContext` modified that test to make it test too much at
once, rather than adding a separate test.

We now have isolated tests for both functions, and also a property test
showing everything put together (in the form of an eta rule for strings
with context). This is better coverage and properly reproduces the bug.

(cherry picked from commit 88d8f6a)
github-actions bot pushed a commit that referenced this issue Jan 23, 2023
@Ericson2314 @github-actions
Fix #7655
af27f2f 
We had some local variables left over from the older (more
complicated) implementation of this function. They should all be unused,
but one wasn't by mistake.

Delete them all, and replace the one that was still in use as intended.

(cherry picked from commit 0afdf40)
roberth added a commit that referenced this issue Jan 23, 2023
@roberth
Merge pull request #7666 from NixOS/backport-7657-to-2.13-maintenance
2c552c6 
[Backport 2.13-maintenance] Fix #7655
@roberth
Copy link
Member Author

roberth commented Jan 26, 2023

The fix has been released in 2.13.2.

iFreilicht pushed a commit to iFreilicht/nix that referenced this issue Jan 28, 2023
@Ericson2314 @iFreilicht
Expand tests to reproduce NixOS#7655
757a7e7 
The original `builtins.getContext` test from
1d75729 would have caught this. The
problem is that b30be6b adding
`builtins.appendContext` modified that test to make it test too much at
once, rather than adding a separate test.

We now have isolated tests for both functions, and also a property test
showing everything put together (in the form of an eta rule for strings
with context). This is better coverage and properly reproduces the bug.
iFreilicht pushed a commit to iFreilicht/nix that referenced this issue Jan 28, 2023
@Ericson2314 @iFreilicht
Fix NixOS#7655
533fbe9 
We had some local variables left over from the older (more
complicated) implementation of this function. They should all be unused,
but one wasn't by mistake.

Delete them all, and replace the one that was still in use as intended.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Ericson2314 Ericson2314

Labels
bug regression Something doesn't work anymore
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #7655 obsidiansystems/nix
2 participants
@roberth @Ericson2314