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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix nix print-dev-env & nix develop with drv paths #8310

Merged
merged 1 commit into from May 10, 2023
Merged

Fix nix print-dev-env & nix develop with drv paths #8310

merged 1 commit into from May 10, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented May 9, 2023 •
edited

Motivation

Fixes #8309

Context

This regression was because both CmdDevelop and CmdPrintDevEnv were switched to be InstallableValueCommand subclasses, but actually neither should have been.

The nixpkgsFlakeRef method should indeed not be on the base installable class, because "flake refs" and "nixpkgs" are not installable-wide notions, but that doesn't mean these commands should only accept installable values.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 馃憤 to pull requests you find important.

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner May 9, 2023 15:54
@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels May 9, 2023
@Ericson2314 Ericson2314 mentioned this pull request May 9, 2023
Closed
@thufschmitt
Copy link
Member

This regression was because both CmdDevelop and CmdPrintDevEnv were switched to be InstallableValueCommand subclasses, but only CmdDevelop should have been.

I'm not sure if I get this. Why should CmdDevelop and CmdPrintDevEnv be different in that regard?

@thufschmitt
Copy link
Member

Case in point, nix develop also suffers from the same issue, and isn't fixed by this PR:

$ nix run github:obsidiansystems/nix/fix-8141 -- develop "$(nix eval --raw nixpkgs#hello.drvPath)"
warning: The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '/nix/store/m7clchh41sryqpiz6x4mbrwhv4n08k3g-hello-2.12.1.drv^*'
error: installable '/nix/store/m7clchh41sryqpiz6x4mbrwhv4n08k3g-hello-2.12.1.drv' does not correspond to a Nix language value
Try '/nix/store/bd3wxql9rbdidghkbwrgyfz6zr5a970z-nix-2.16.0pre20230509_55e3715/bin/nix --help' for more information.

@Ericson2314
Copy link
Member Author

Oh, I thought nix develop never worked on plain derivations, but yes if I am mistaken about that there's a simpler fix.

@Ericson2314
Copy link
Member Author

Oops, yes that is indeed the case.

@Ericson2314
Fix nix print-dev-env & nix develop with drv paths
a93110a 
Fixes NixOS#8309

This regression was because both `CmdDevelop` and `CmdPrintDevEnv` were
switched to be `InstallableValueCommand` subclasses, but actually
neither should have been.

The `nixpkgsFlakeRef` method should indeed not be on the base
installable class, because "flake refs" and "nixpkgs" are not
installable-wide notions, but that doesn't mean these commands should
only accept installable values.
@Ericson2314 Ericson2314 changed the title Fix nix print-dev-env with drv paths Fix nix print-dev-env & nix develop with drv paths May 10, 2023
@Ericson2314
Copy link
Member Author

OK nix develop is fixed now too.

thufschmitt
thufschmitt approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks, that looks great!

@thufschmitt thufschmitt merged commit f60b215 into NixOS:master May 10, 2023
10 checks passed
@Ericson2314 Ericson2314 deleted the fix-8141 branch May 10, 2023 18:49
@blaggacao
Copy link
Contributor

Thanks a lot for the expedited fixing! Can we back-port this for 2.15, so that installers such as DetSys Installer would not default to a unfixed version?

@Ericson2314 Ericson2314 added the backport 2.15-maintenance Automatically creates a PR against the branch label May 11, 2023
@Ericson2314
Copy link
Member Author

Added the label to do so.

@github-actions
Copy link

Successfully created backport PR for 2.15-maintenance:

@github-actions github-actions bot mentioned this pull request May 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thufschmitt thufschmitt thufschmitt approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees
No one assigned
Labels
backport 2.15-maintenance Automatically creates a PR against the branch new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

broken nix print-dev-env on drvs
3 participants
@Ericson2314 @thufschmitt @blaggacao