Enhancements for shell plugins #698

bfloch · 2019-08-26T16:34:38Z

This fixes and improves the shell plugins, especially on Windows for cmd and PowerShell-like.
Formally excluded shell dependent tests are now passing.

I would love to have someone run the test-suite on MacOS.

Future work

Better Visual Studio handling in CMake for Windows
Remove dependency of pwsh on SH-class for get_syspaths under Linux

Fixes

Rex and expandable in other shells #694 Expand both generic environment variables $ENVVAR / ${ENVVAR} as well as shell specific ones correctly, like %ENVVAR% ${Env:ENVVAR} for cmd and PowerShell-like shells
Quotation marks issues on Windows. #691 Undoes regression in quotes handling of cmd

Features / Enhancements

Added get_all_key_tokens to allow for multiple key-forms
Added convert_tokens to normalize token forms to shell specific form
Allow pwsh shell on Linux (and pass tests)
Implements optional script execution on windows via PATHEXT.
- Windows shells may add custom PATHEXT values in plugin config file
- create_executable_script now returns list of created scripts based on config.create_executable_script_mode:
  - single (default) Just create the requested script. Usually extension-less.
  - py create .py script that will allow launching scripts on windows,
  - platform_specific will create .py script on windows and requested on other platforms
  - both creates the requested file and a .py script for windows
Introduced Shell specific regex parsing to handle ambiguous case like set PATH=$ENV:VAR in PowerShell vs. Bash within the NamespaceFormatter
Introduced printout of shell name for shell_dependent test failure

Tests passing

Backwards compatibility

Fully backwards compatible, new features use config options default to current behaviour
Shell Plugin API has been extended slightly.
- Implement get_all_key_tokens(key) (with safest/common form at index 0)
- Implement line_terminator() for shell/platform specific line_terminator as expected by the shell
- join now a class method
- Extend ENV_VAR_REGEX if needed with shell specific token forms
- You can make use of new convert_tokens in you implementation of info and error

src/rezplugins/shell/cmd.py

src/rez/rex.py

src/rez/rezconfig.py

src/rez/shells.py

src/rez/tests/test_shells.py

src/rez/util.py

src/rezplugins/shell/cmd.py

src/rezplugins/shell/powershell.py

src/rezplugins/shell/pwsh.py

src/rez/bind/hello_world.py

bfloch · 2019-08-29T14:20:11Z

Thanks for the review. I'll dropped notes/questions and will look into the fixes.

bfloch · 2019-08-29T19:39:12Z

I've updated the first post and addressed everything in your review.
One thing I could not solve is the SH reference in pwsh, but I limited the scope and added a proper comment with a reference how this might be solved upstream of pwsh.

Ran the tests again, pass as before.

maxnbk · 2019-08-30T21:22:01Z

FYI, zsh tests on master should be fixed when #711 is merged in.
Notifying in case it matters for this PR.

src/rez/shells.py

nerdvegas · 2019-08-31T04:04:31Z

I'm ready to merge this, but would like to hear RE OSX testing also. In the meantime I'm going to start looking at GitHub Actions, so we'll have this testing covered anyway (and across py2/3 also, which is going to become important pretty soon).

src/rezplugins/shell/cmd.py

bfloch · 2019-09-05T13:51:54Z

FYI, zsh tests on master should be fixed when #711 is merged in.
Notifying in case it matters for this PR.

Cool will try merging this in. Thanks!

bfloch · 2019-09-05T13:54:52Z

Will fix the conflict and I'll give the de-duplication on cmd another shot today.
Might have to do with shell=False again, not sure. The syntax certainly works within cmd sessions ...
EDIT: Got it to work with cmd. Only thing left is rebasing on latest master which should also have the zsh changes.

…all shell tests. This implements ${VAR} and $VAR variables for cmd and Powershell like, as well as their native forms like %VAR% and $Env:VAR. In order to handle the ambiguity of variables in the form of $Env:Literal in Unix and Windows the NamespaceFormatter may take interpreter regex into account that is being supplied by the underlying shell. For command execution on Windows .PY is being added to the PATHEXT and the create_script function is extended to create any form of execution script. This behaviour can be controlled via a rezconfig, but defaults to backwards compatible Unix-only behaviour.

This introduced quotation issues as detailed in AcademySoftwareFoundation#691 This reverts commit e513b69. # Conflicts: # src/rezplugins/shell/cmd.py

It should be noted that according to [1] this is not an officially exposed API, but in order to avoid License clearance for now I just used it similar like we use pipes.quote in `shells.py`. [1] https://bugs.python.org/issue10838

Line termination is not necessarirly platform dependent, but also shell dependent.

Shells which do not use the generic ${VAR} or $VAR form should use convert_token to convert to their native form. This only assumes that a shell correctly extends ENV_VAR_REGEX to also parse their own variables.

Also fixes spelling of attribute to conform to PowerShell style.

bfloch · 2019-09-06T17:19:41Z

FYI, zsh tests on master should be fixed when #711 is merged in.
Notifying in case it matters for this PR.

Notable changes since your last review:

Fixed promp not expanded with cmd (probably because variable expansion now works properly)
Supressing PowerShell header
Added PATHEXT deduplication for cmd and PowerShell as suggested
Rebased on current master. Fixed the zsh failure as advertised. Thanks.

So now it is just a question if we want #707 first.
I guess since it technically does not rely on it and we do have quite a lot of changes here we should probably go ahead and merge it. At least we get some real feedback early on, while there still might be some design discussions happening in #707.

All tests (except cmake related) pass on Win & Linux.
MacOS testing is dependent on the github actions setup, I presume. I'll leave that part to you guys since I don't have access to a Mac.

nerdvegas · 2019-09-06T21:17:37Z

Thanks Blazej. I'm happy to merge this and will do soon - it stands on its own from #707 and fixes an outstanding bug on Windows.

nerdvegas · 2019-09-10T23:15:37Z

Just an update - getting to this soon. We're near the end of the py3 work, I'm hoping to get that done first then merge this right after.

bfloch · 2019-09-11T01:03:10Z

No worries, there is no rush with this merge. I would love to hear your feedback on #737 though since this is the last missing piece for our Rez deployment.

Btw. I haven't tested this PR with python 3, so let me know if there is any work to be done.
In fact, we as a Blender-based animation studio would love to run rez with python 3 so I might be running tests based on your effort soon.

nerdvegas · 2019-09-11T01:10:19Z

Yeah I'm pretty slammed atm but I'm keen to get onto both #737 and #703 so I'll do that soon. We're maybe a few days/a week away from wrapping up on py3 port, so I reckon if you could do some testing on it once that's done, that would be very helpful. Cheers A

…

On Wed, Sep 11, 2019 at 11:03 AM Blazej Floch ***@***.***> wrote: No worries, there is no rush with this merge. I would love to hear your feedback on #737 <#737> though since this is the last missing piece for our Rez deployment. Btw. I haven't tested this PR with python 3, so let me know if there is any work to be done. In fact, we as a Blender-based animation studio would love to run rez with python 3 so I might be running tests based on your effort soon. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#698>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSQHJLGVHIA4USJAXMTQJA7VBANCNFSM4IPRPMLQ> .

bfloch commented Aug 26, 2019

View reviewed changes

src/rezplugins/shell/cmd.py Outdated Show resolved Hide resolved