[Feature] Additional tools to Path #661
Conversation
… PATH Shells now have the option not only to replace the $PATH with standard_system_paths, but also to append or prepend to it. This is controlled with the new standard_system_paths_visibility config variable that can be set to: - replace (default) - prepend - append
This optimization was originally handled within the Shell, but the dependency on configuration behaviour should not be really the responsibility of the implementer. Note that caching (cls.syspaths) is still responsibility of the Shell implementation, since we shall not reason about it.
…d to PATH Note how the config change is not part of the cache, which is what you want.
bfloch
force-pushed
the
feature/issue_660_additional_path
branch
from
210df34 to
9b05b24
Compare
|
Tests passing:
|
|
Thanks Blazej, will get on this soon. Ignore previous email I didn't see
you had the PR in already!
Cheers
A
…On Fri, Jul 12, 2019 at 6:38 AM Blazej Floch ***@***.***> wrote:
Tests passing:
- Windows 10.0.17134
- cmd
- Linux Ubuntu 18.04.2 LTS (WSL)
- tcsh
- sh (fails on test_rex_code_alias, possibly other bug,
investigating)
- bash
- csh
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#661>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSTPUUVX3EXSEILQW2DP66K4TANCNFSM4IBRVEUQ>
.
|
|
Btw. @nerdvegas: |
bfloch
changed the title
bfloch
changed the title
In theory, the |
|
Hey @bfloch this in on my radar, but I'm not sure I'll be able to get to it until after SIGGRAPH, just FYI. |
|
No worries. I'll have it more tested next week and try to make the tests cleaner. |
| @@ -68,14 +68,11 @@ def get_startup_sequence(cls, rcfile, norc, stdin, command): | |||
| ) | |||
|
|
|||
| @classmethod | |||
| @syspaths_composer | |||
There was a problem hiding this comment.
So the functionality of syspaths_composer is fine, however I don't think it's correct for it to be a decorator like this.
The intent of Shell.get_syspaths is to define what system paths are associated with that shell - not how to then apply them. So I would just take the functionality from the decorator, and apply it only where syspaths are actually set into PATH (https://github.com/nerdvegas/rez/blob/master/src/rez/rex.py#L1124)
|
This is just a comment - much as mentioned in #698, this feature may be unnecessary when we implement better env-var management (as described in #707). I don't want to hold back this PR to wait for that, but I think it's worth noting that this could become deprecated in lieu of that at some point (which would be good, because it means less edge cases). |
| @@ -366,10 +366,18 @@ | |||
| # scripts (such as .bashrc). If False, package commands are sourced after. | |||
| package_commands_sourced_first = True | |||
|
|
|||
| # Defines how the standard_system_paths configuration acts on the $PATH: | |||
| # - "replace": Replace $PATH if standard_system_paths is set | |||
| # - "append": Append standard_system_paths to determined $PATH | |||
There was a problem hiding this comment.
This is confusing, I think you mean:
Append 'standard_system_path' to automatically determined system paths
..right? Because the comment mentions $PATH, it's confusing because it seems like this is just getting appended to the entirety of $PATH. It's not clear that you mean the initially determined $PATH.
There was a problem hiding this comment.
Ok ignore that, I think I have an idea on how to make this clearer. It's jut inherently a little hard to explain I think.
| @@ -271,6 +278,7 @@ def _parse_env_var(self, value): | |||
| "plugin_path": PathList, | |||
| "bind_module_path": PathList, | |||
| "standard_system_paths": PathList, | |||
| "standard_system_paths_visibility": StandardPathVisibility_, | |||
There was a problem hiding this comment.
I think "standard_system_paths_mode" would be more intuitive.
| # Defines paths to initially set $PATH to, if a resolve appends/prepends $PATH. | ||
| # If this is an empty list, then this initial value is determined automatically | ||
| # depending on the shell (for example, *nix shells create a temp clean shell and | ||
| # get $PATH from there; Windows inspects its registry). | ||
| # The way the list is combined with $PATH is controlled via | ||
| # rez_standard_system_paths_visibility. |
There was a problem hiding this comment.
Instead:
# ...$PATH from there; Windows inspects its registry).
#
# To specify both your own systems paths, AND automatically determined system paths, use the
# "standard_system_paths_visibility" setting. For example, setting this to "append" will initially
# set $PATH to the automated system paths, followed by the paths listed here.
|
I agree. Let's focus on #707. |
Implements #660.
Compatibility
replacestandard_system_pathsvariable usageget_syspathswithsyspaths_composerto make use of the new feature.What do we gain?
standard_system_pathsout of plugin implementation into central decoratorsyspaths_composer