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

Get rid of the --derivation flag #8594

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Get rid of the --derivation flag #8594

wants to merge 2 commits into from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jun 27, 2023 •
edited
Loading

Motivation

Since PR #7601, we've had enough flexibility in our types of installables that it is no longer needed. Almost. The only remaining issue is that drvPath has the wrong type of string for historical reasons, see #7910.

Context

Progress towards #7261

depends on #9216

Test plan

The test plan is taken by Robert's comment in #7910 (comment) describing how we could migrate Nixpkgs without a breaking change in Nix. The Nix testsuite has its own mkDerivation, and we so we do the same thing with it:

  • drvPath is now overridden to not have the funky DrvDeep string context anymore.

  • Tests that previously needed to builtins.unsafeDiscardOutputDependency a drvPath no don't.

  • Tests that previous did not need to builtins.unsafeDiscardOutputDependency a drvPath now do.

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.

@github-actions github-actions bot added new-cli Relating to the "nix" command tests labels Jun 27, 2023
@Ericson2314 Ericson2314 force-pushed the derivation-flag-bye branch from 4f712fe to 2f3e9e9 Compare June 27, 2023 16:29
@roberth
Copy link
Member

roberth commented Jun 27, 2023

depends on #8595

Idealistically yes, but for practicality, shouldn't we make the CLI do the expected thing for that weird string context, as a backcompat measure for older Nixpkgs?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jun 27, 2023 •
edited
Loading

Hmm I dunno. I don't like how it wouldn't respect (at least my notion of) "referential transparency". I do admit it's not hard to do that, but I rather start with the cleaner thing, and only add the other if people absolutely demand it. Can always add it later, but can't remove it once added (and stable).

But maybe I am being unreasonable and I just dislike DrvDeep too much? If it warns in the DrvDeep case will that help remind people to switch to the new way of doing things? Or is that obnoxious noise when no on is empowered to e.g. change old Nixpkgs anyways?

@Ericson2314 Ericson2314 mentioned this pull request Jul 14, 2023
Open
@roberth
Copy link
Member

roberth commented Jul 14, 2023

I don't like how it wouldn't respect (at least my notion of) "referential transparency".

In what way is it violated?

If it warns in the DrvDeep case will that help remind people to switch to the new way of doing things?

A way to drive a wedge between those who need the old behavior of .drvPath and those who want a warning free Nixpkgs.
Ok, I'm exaggerating, but we should really make #8595 widely available before even considering this.

@Ericson2314 Ericson2314 force-pushed the derivation-flag-bye branch from 2f3e9e9 to 8c18ed7 Compare July 31, 2023 17:12
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 31, 2023
@Ericson2314 Ericson2314 force-pushed the derivation-flag-bye branch 2 times, most recently from 05f4679 to 248993d Compare October 23, 2023 16:02
@Ericson2314
Flip default in testsuite mkDerivation
d34c1c3 
The test plan is taken by Robert's comment in
NixOS#7910 (comment)
describing how we could migrate *Nixpkgs* without a breaking change in
Nix. The Nix testsuite has its own `mkDerivation`, and we so we do the
same thing with it:

 - `drvPath` is now overridden to not have the funky `DrvDeep` string
   context anymore.

 - Tests that previously needed to
   `builtins.unsafeDiscardOutputDependency` a `drvPath` no don't.

 - Tests that previous did *not* need to
   `builtins.unsafeDiscardOutputDependency` a `drvPath` now *do*.
@Ericson2314
Get rid of the --derivation flag
ff207e6 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
@dpulls
Copy link

dpulls bot commented Oct 23, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 changed the base branch from master to 0.5-release October 23, 2023 17:28
@Ericson2314 Ericson2314 changed the base branch from 0.5-release to master October 23, 2023 17:28
@Ericson2314 Ericson2314 marked this pull request as ready for review October 23, 2023 17:31
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner October 23, 2023 17:31
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-10-27-nix-team-meeting-minutes-98/34695/1

@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting 2023-11-10:

  • Idea: replace --derivation with .drvPath
    • Currently doesn't work because .drvPath has too big of a string context. And we can't really change that as that would be a breaking change
    • Plan: keep .drvPath the same for Nix, just change it in Nixpkgs
  • @regnat: .drvPath is not a great interface. Something like .derivation would already be better
  • @edolstra: the current --derivation flag is more discoverable
  • @roberth: but --derivation only makes sense if the build plan is simple (no drvs-that-generate-drvs)
  • @Ericson2314: Also, --derivation is global, not per-installable. So you can't mix-and-match

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-10-nix-team-meeting-minutes-102/35379/1

@tomberek
Copy link
Contributor

tomberek commented Dec 8, 2023 •
edited
Loading

Did a review of use-cases via https://sourcegraph.com/search?q=context:global+nix.*--derivation&patternType=standard&sm=1&groupBy=repo to see if there are situations not covered by the proposal. The one that seems to be needed is nix path-info --derivaiton being used to obtain the build-time closure.

Top 4

  • nix why-depends --derivation
  • nix copy --derivation
  • nix path-info --derivation
  • nix-tree --derivation

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-12-08-nix-team-meeting-minutes-110/36721/1

@Ericson2314 Ericson2314 marked this pull request as draft January 19, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Assignees
No one assigned
Labels
new-cli Relating to the "nix" command tests 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.
5 participants
@Ericson2314 @roberth @nixos-discourse @fricklerhandwerk @tomberek