-
Notifications
You must be signed in to change notification settings - Fork 337
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.
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
Rex and expandable in other shells #694
Comments
Got feedback from @nerdvegas in Slack that this is incorrect behavior. As such I wonder if it is even necessary to introduce backwards compatibility options. |
Actually I found a duplicated reported by myself some time ago: |
…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.
…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.
So yes, I think we need to support |
I have a branch which fixes this and many other shell related issues. This allowed to pass all shell tests cross platform. Still having some issues with zsh which might not be related to it, but I'll investigate before creating a PR. |
…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.
…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 applies to any version up to 2.40.3 as far as I can tell.
Rex does
expandvars("${VAR}")
correctly regardless of the platform:https://github.com/nerdvegas/rez/blob/master/src/rez/utils/formatting.py#L203
However, the default
expandable
will only work on shells that happen to support the${VAR}
or$VAR
syntax.On windows the tests check for
%VAR%
but in fact the documentation make it sound as if the${VAR}/$VAR
is the portable form that should work on any platform.I assume that we should implement the necessary conversion for the expandable case on shells that require it.
So for cmd.exe we would replace
${VAR}
with%VAR%
and for Powershell we would replace it with${Env:VAR}
.This way we can keep the package definitions platform independent. The risk is minimal since users are free to choose
literal
.The question arises is expandvars should also replace the variables in the case where the variable is not found in the environment.
In both cases we might want to introduce rez configuration options for backwards compatibility.
The text was updated successfully, but these errors were encountered: