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

docs: fix nix-shell commands #7849

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Feb 16, 2023

generalize example commands for nix-shell

why

im using man nix-shell as a cheatsheet for

nix-shell '<nixpkgs>' -A some_package
eval ${unpackPhase:-unpackPhase}
cd $sourceRoot
eval ${patchPhase:-patchPhase}
eval ${configurePhase:-configurePhase}
eval ${buildPhase:-buildPhase}

${patchPhase:-patchPhase} was missing

cd $sourceRoot is more generic

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
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

This whole example should probably not even be here, because all of it is specific to Nixpkgs. I'll merge this for the example to be correct, but eventually it should find a new home in the Nixpkgs manual.

Would be great if you could take the time to make a PR there and ping me.

@edolstra
Copy link
Member

edolstra commented Feb 17, 2023

IMHO since this is a specific example, it doesn't have to be generic. E.g. if Pan doesn't have a patchPhase, then there is no need to run the patchPhase.

@fricklerhandwerk I don't think it's a problem that it's specific to Nixpkgs, since that's what everybody uses. (nix-shell even has a hard dependency on Nixpkgs...)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-44/25546/1

@fricklerhandwerk
Copy link
Contributor

@edolstra I still think we should just remove that part here because it doesn't really help at this point.

I also fear the entangling may actually get worse by codifying it in documentation.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-45/26397/1

@edolstra edolstra disabled auto-merge April 3, 2023 12:30
@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-03-31-nix-team-meeting-minutes-45/27002/1

@fricklerhandwerk
Copy link
Contributor

Discussed in the Nix team meeting 2023-04-03:

  • @fricklerhandwerk: Nixpkgs manual now contains the exact same section
  • @thufschmitt: should we add a link to the Nixpkgs manual referring to stdenv?
    • @roberth: we could just replace this example by the link
    • @edolstra: the Nix manual shows how to use nix-shell
    • @fricklerhandwerk: should remove the example from this manual altogether, since it's about interacting with stdenv, which is completely documented in Nixpkgs
      • @edolstra: fundamentally against this; the manual should show real-world examples
        • @fricklerhandwerk: yes. the point here is that the example is not relevant to nix-shell but stuff that's going on in the realm of Nixpkgs
        • @edolstra: no, examples should demonstrate typical use cases, and that is one
  • @Ericson2314: we should discuss the layer violation separately
    • (some discussion, anyway, without agreement)
    • postponed going into further depth
    • @Ericson2314: the way out would be demonstrating that a (hypothetical) nixpkgs shell that sidesteps the layer violation can work just fine
  • decision: merge

@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-04-03-nix-team-meeting-minutes-46/27008/1

@fricklerhandwerk fricklerhandwerk merged commit bbdb5a5 into NixOS:master Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants