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

reword documentation on nix-path config option #7772

Merged
merged 2 commits into from
Jul 31, 2024
Merged

reword documentation on nix-path config option #7772

merged 2 commits into from
Jul 31, 2024

Conversation

fricklerhandwerk
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk commented Feb 7, 2023 •
edited
Loading

also make the fallback values explicit.

Motivation

this is to clarify behavior. using the positive boolean is easier to read,
as it does not require flipping bits in your head.

Context

Follow-up on #7689 by @ncfavier instead of making a post-factum review.

Update: unfortunately that PR did not actually implement the desired behavior.

Depends on: #7871

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 bug fix: updated release notes

edolstra
edolstra reviewed Feb 8, 2023
View reviewed changes
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
@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 fricklerhandwerk mentioned this pull request Mar 5, 2023
Closed
7 tasks
@Ericson2314
Copy link
Member

Or we trying to document the current behavior or the intended behavior? Because this to me is not quitethe intended behavior.

The rules I wrote in #7871 (comment) are not intended to be just just for nixPath but for any such configuration variable. The rules around settings files, NIX_CONF, --extra-foo, and --foo are entirely standard and additionally rules for NIX_FOO go for many other such configuration options that also have their own env vars.

Documenting this stuff per a specific option like nixPath implies that it is somehow special. It is today because of the weirdness of "restricted eval" that is in conflict with these general rules. But we should fix "restricted eval"` so as to not break those rules instead. Then we can restore the general pattern and not have special cases.

@fricklerhandwerk
Copy link
Contributor Author

I agree this should just follow the standard pattern. But since there are four possibilities of changing the search path, I opted for making that explicit here. We don't have to do it that way, and I'm open for concrete suggestions.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 6, 2023 •
edited
Loading

@fricklerhandwerk I am thinking of adding to BaseSettings an optEnvVar. And then we can programmatically document this for all of them! :)

(Or maybe it would be sub-class.)

@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

@fricklerhandwerk
Copy link
Contributor Author

Triaged in the Nix team meeting 2023-03-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-03-31-nix-team-meeting-minutes-45/27002/1

@fricklerhandwerk fricklerhandwerk mentioned this pull request Apr 17, 2023
Open
2 tasks
@fricklerhandwerk fricklerhandwerk marked this pull request as draft May 3, 2023 09:43
@fricklerhandwerk fricklerhandwerk mentioned this pull request Jul 5, 2024
Open
@dpulls dpulls
Copy link

dpulls bot commented Jul 24, 2024

🎉 All dependencies have been resolved !

@github-actions github-actions bot added documentation with-tests Issues related to testing. PRs with tests have some priority labels Jul 25, 2024
@fricklerhandwerk fricklerhandwerk marked this pull request as ready for review July 25, 2024 01:41
@fricklerhandwerk
docs: unify documentation on search paths
cdcb99f 
- put all the information on search path semantics into `builtins.findFile`
- put all the information on determining the value of `builtins.nixPath` into the
  `nix-path` setting

  maybe `builtins.nixPath` is a better place for this, but those bits
  can still be moved around now that it's all next to each other.
- link to the syntax page for lookup paths from all places that are
  concerned with it
- add or clarify examples
- add a test verifying a claim from documentation
@fricklerhandwerk fricklerhandwerk merged commit db5bacb into NixOS:master Jul 31, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@ncfavier ncfavier ncfavier approved these changes

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

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

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

Assignees
No one assigned
Labels
documentation 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
@fricklerhandwerk @nixos-discourse @Ericson2314 @edolstra @ncfavier