Implement #1196

[instinct-vfx]

This is a work in progress PR to implement #1196. Initial prototype done by @davidlatwe has been integrated here as a start.

[instinct-vfx]

I need to clean up the code because i think it uses an older version of the plugin as base (still using $Env instead of Set-Item). Will do so today and try to move this out of WIP status. This actually solves another issue for me where on some Machines rez fails to get_syspath when already in a rez created shell. This seems to also fix this issue.

[instinct-vfx]

Added License text back and removed the now unused ENV_VAR_REGEX

[instinct-vfx]

Updated with current master

[instinct-vfx]

So i consider this ready to be merged now. Thanks again @davidlatwe for helping find the remaining issue (see last commit reverting one of the changes). Tests are all green now, in addition to the Actions all running successfully i also checked locally that tests run fine with python 3.x.

[nerdvegas]

Hey Thorsten,

Any chance we can get a TL;DR on this? I'm still not quite sure of the whole picture on this one, and info seems to be fairly scattered.

I'm specifically not clear on the ENV_VAR_REGEX override. This regex is used to convert env-var refs in package commands (ie non-shell-specific) into shell-specific representation (via convert_tokens). However, (and confusingly) the docstring on ENV_VAR_REGEX suggests it may be extended to support shell-specific forms.

I just want to be sure that this fix isn't a fix for the wrong reasons, if you see what I mean?

Thx
A

[instinct-vfx]

Hey Allan,

yes totally and sorry for the lack of context. The ENV_VAR_REGEX is not introduced by this PR. I had removed it because i thought it was not needed anymore after the changes to get_syspaths (i was not aware this was also used in shells.py for example).

The TL;DR is changing get_syspaths to use PowerShell to get the PATH variable contents.

There are two reasons for this: First, the old implementation used the REG command line tool that ships with windows and then parses the result. On the one hand this is less stable than the new implementation because the output of REG is not directly parseable and as you can tell by the RegEx there are at least two different variants that are accounted for already.

Secondly - and the bigger issue - this actually breaks in certain constellations when you call rez from a rez shell (in PowerShell / PowerShell Core). This is because it is not a built in command of cmd.exe or PowerShell but an actual executable (like doskey for the alias command in cmd.exe).

In addition i think this will also solve an issue that Chad Vernon reported on Slack where in their environment using REG is blocked by IT whereas the PowerShell commands worked (at least directly in a shell).

In regards to ENV_VAR_REGEX I think this is beyond the scope of this PR to introduce any changes there so i reverted it to the state it was before and now the only change is get_syspaths

[nerdvegas]

Ah right yes, sorry I did check ENV_VAR_REGEX in master and yes it was already there, I didn't see it and thought it had been added in this PR.

I agree that it's outside the scope of this PR, but I'm left wondering why we did this. Let's address this in #1233

[nerdvegas]

I think we could collapse this into a loop over the two different powershell calls and get rid of some code here

[instinct-vfx]

@nerdvegas Also a good point, i changed that, please check again if the new version is okay for you

[nerdvegas]

see inline comments

[nerdvegas]

oh I think maybe you accidentally reintroduced the shell=True when refactoring into a loop?

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[instinct-vfx]

You are totally right. It's 1am already, i should have pushed this to tomorrow i guess. sigh I THINK i got it all corrected now, tests are still passing and formatting is fixed too. Sorry for the additional loops.