Skip to content

Change NU_LIB_DIRS not to depend on $nu.config-path#8887

Merged
kubouch merged 3 commits intonushell:mainfrom
ito-hiroki:bugfix/8870/nu-lib-dirs
Apr 14, 2023
Merged

Change NU_LIB_DIRS not to depend on $nu.config-path#8887
kubouch merged 3 commits intonushell:mainfrom
ito-hiroki:bugfix/8870/nu-lib-dirs

Conversation

@ito-hiroki
Copy link
Copy Markdown
Contributor

Description

fixed #8870
In default_env.nu, environment variable NU_LIB_DIRS is derived from $nu.config-path now. If the config file is changed (e.g., by using --column), NU_LIB_DIRS is also changed.
This PR adds default-config-dir to $nu and fix default_env.nu to use default-config-dir for NU_LIB_DIRS definition.

User-Facing Changes

Users can use $nu.default-config-dir to specify the default config directory.

Tests + Formatting

$ use toolkit.nu  # or use an `env_change` hook to activate it automatically
$ toolkit check pr

- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :green_circle: `toolkit test`
- :green_circle: `toolkit test stdlib`

After Submitting

nothing

@ito-hiroki ito-hiroki changed the title Change NU_LIB_DIRS not to depend on config-path Change NU_LIB_DIRS not to depend on $nu.config-path Apr 14, 2023
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 14, 2023

I'm fine with following @kubouch's lead here but this seems weird to me, even after following the other conversations.

  1. If someone is specifying a --config /some/alternate/config.nu it seems reasonable to think that all their config settings, including NU_LIB_DIRS, would be based off of that new path, instead of "hard-coded" to the default path.
  2. Having said that, it's easy enough to change the env.nu to point to $nu.config-path instead of $nu.default-config-path

@ito-hiroki
Copy link
Copy Markdown
Contributor Author

ito-hiroki commented Apr 14, 2023

If someone is specifying a --config /some/alternate/config.nu it seems reasonable to think that all their config settings, including NU_LIB_DIRS, would be based off of that new path, instead of "hard-coded" to the default path.

I agree with you.
Following your comment, I think that only adding default-config-path is enough in this PR. Should we change NU_LIB_DIRS in default_env.nu?
(I forgot to commit my test changes. I will add it.)

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 14, 2023

It's not only the --config flag but also symlink resolving that changes the config-path. If you have a config symlinked in your default config directory, the config-path will point at different location than the default directory. In that case you still want to refer to the default directory. I'm thinking having a default static NU_LIB_DIRS would make the default behavior more predictable, you can always easily change in your config.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2023

Codecov Report

Merging #8887 (4f332ef) into main (4a8124a) will increase coverage by 0.37%.
The diff coverage is 93.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8887      +/-   ##
==========================================
+ Coverage   68.12%   68.50%   +0.37%     
==========================================
  Files         635      635              
  Lines      101520   101534      +14     
==========================================
+ Hits        69163    69551     +388     
+ Misses      32357    31983     -374     
Impacted Files Coverage Δ
crates/nu-engine/src/nu_variable.rs 40.20% <93.33%> (+3.53%) ⬆️

... and 2 files with indirect coverage changes

@sholderbach
Copy link
Copy Markdown
Member

Is this a breaking change?

@ito-hiroki
Copy link
Copy Markdown
Contributor Author

Users using a custom config directory will be affected when they replace their env.nu to default_env.nu. One option is not to change default_env.nu to respect the existing configuration, but which is the best?

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 14, 2023

I'd mark it as a breaking change because it changes default env.nu. But actually, what this PR does is that it brings the defalt behavior back after #8792 which caused the breaking change.

@kubouch kubouch added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Apr 14, 2023
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 14, 2023

@fdncred Are you OK with this PR or do you still have some concerns?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 14, 2023

@kubouch I'm good if you're good.

@kubouch kubouch merged commit 45d33e7 into nushell:main Apr 14, 2023
@sholderbach
Copy link
Copy Markdown
Member

I'd mark it as a breaking change because it changes default env.nu. But actually, what this PR does is that it brings the defalt behavior back after #8792 which caused the breaking change.

I don't see how #8792 should have changed the NU_LIB_DIRS behavior. It only affects the behavior of config nu and config env?

Might this be related to one of the IDE PRs?

bobhy pushed a commit to bobhy/nushell that referenced this pull request Apr 15, 2023
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 17, 2023

@sholderbach This line used to depend on $nu.config-path which became volatile after the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NU_LIB_DIRS environment variable no longer working

4 participants