-
Notifications
You must be signed in to change notification settings - Fork 338
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
Better Windows Support #377
Conversation
…l plugins to use the alternate library on windows to enable git and command hook support on windows.
… support interactive environment creation in subshells.
…when running in non-interactive command mode.
How do these fixes relate to this PR? #341 There seems to be quite some overlap. In regards to pbs and sh i opted to remove the dependency altogether as it was only used for the export method anyways. Could you share why you went for pbs instead? |
I think the end result would be about the same, the main difference I can see is how we deal with interactive shells. I omit adding the There is a comment about a rez code exec operation that passes an empty string command. I'd have to check on that because I believe that would get treated as a interactive shell in my implementation, which may not be correct. PR #341 also adds some features that my PR doesn't address
And my PR addresses theses issues that aren't part of #341
I think these changes could be merged with the exception of the interactive shell implementation, I'd have to do more research on that to see which way makes more sense.
I only chose |
Thanks a lot for the in-depth explanation! I simply removed the import, and the method. The only reason the method can not use the same implementation as the rest of the git commands in the module is, that it is marked to be a classmethod and hence can not use self. I am unsure if this is needed or if it maybe should live elsewhere, or construct it's own command helper. |
This fixes interactive shells for me on windows, but not #348 Also, I was a little surprised that running rez-env from a git-bash environment opened up a cmd.exe subshell. I suppose this is the expected behavior? |
Yeah, git bash is a different shell. It looks like rez is currently hardcoded to only use the You could add some logic there to figure out what the correct current shell is on windows, then add a plugin to work with the git-bash shell (might just be able to repurpose most of the unix bash shell plugin). |
Interested and confused a bit too here. I would love to see the different overlapping PRs for windows fixed once and for all. There is still some other areas to work on, but fixing this issue along with fixing the git plugin ( #460 ) would be the minimal base to start deploying master here. |
It looks like you already fixed the git issue, right? Is #460 ready to be merged? If you do that, I can pull it down and update this PR to be a cleaner update that just addresses the windows issue.
I don't see any other open PR's dealing with Windows support. The only other one I remember was #341, which has already been merged, but doesn't fix the windows interactive-shell issue. If you know of other overlapping PR's, I can definitely take a look at them and merge those changes into this PR. |
@@ -482,7 +482,7 @@ def symlink(self, source, link_name): | |||
raise ctypes.WinError() | |||
|
|||
def _terminal_emulator_command(self): | |||
return "CMD.exe /Q /K" | |||
return "START" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"terminal emulator" is probably a bit misleading. This command is only used when launching a rez environment using detached=True
. CMD.exe
just spawns a subshell within the current shell. START
will launch a separate shell.
Righto, I'm on this today. |
Several fixes and improvements
pbs
package to vendor libraries and updated the git vcs plugin and release hook so thatrez-release
will work on windows with git./K
flag instead of/C
flag) when run in interactive mode. Non-interactive mode still uses the/C
flag so that the shell exits when the non-interactive command exits.detached
is handled in windows shells to useSTART
instead ofCMD /Q /K
. The former will cause windows to open a new shell, which is what I believe is the purpose ofdetached
.Primarily a fix for #375 but it may solve other tickets as well.