Skip to content
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

Merged
merged 15 commits into from
Nov 29, 2017
Merged

Conversation

bpabel
Copy link
Contributor

@bpabel bpabel commented Jan 26, 2017

Several fixes and improvements

  • Added a patched version of the pbs package to vendor libraries and updated the git vcs plugin and release hook so that rez-release will work on windows with git.
  • Added pycharm project files to .gitignore
  • Updated the windows cmd shell plugin to keep the shell open (via the /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.
  • Updated the way that detached is handled in windows shells to use START instead of CMD /Q /K. The former will cause windows to open a new shell, which is what I believe is the purpose of detached.
  • Updated the windows cmd shell plugin to correctly write comments and join multiple commands.

Primarily a fix for #375 but it may solve other tickets as well.

Brendan Abel added 5 commits January 12, 2017 16:18
…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.
@instinct-vfx
Copy link
Contributor

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?

@bpabel
Copy link
Contributor Author

bpabel commented Jan 26, 2017

How do these fixes relate to this PR? #341 There seems to be quite some overlap.

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 exit command, whereas #341 keeps the exit command for both interactive and non-interactive shells, but launches an additional subshell on line 160 in interactive mode, and that shell gets exited by the exit command rather than the interactive shell. The results should be mostly the same, but with an additional shell being launched in interactive mode, if there are autorun scripts that run on shell launch, they're going to be run twice in the #341 implementation.

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

  • Character escaping fixes
  • Alias fixes.
  • Defining default system paths in the config file.

And my PR addresses theses issues that aren't part of #341

  • Detached support for cmd shells.
  • Fixes for cmd comments
  • Fixed git support for rez-release

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.

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 only chose pbs because sh isn't supported on windows. I even had to patch pbs to get it to work with the way it was being used (from pbs import git). I'm fine with dropping the dependency altogether. I just needed the plugin to import to use rez-release in windows.

@instinct-vfx
Copy link
Contributor

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.

@assumptionsoup
Copy link

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?

@bpabel
Copy link
Contributor Author

bpabel commented Feb 9, 2017

Yeah, git bash is a different shell. It looks like rez is currently hardcoded to only use the cmd shell plugin on windows - https://github.com/nerdvegas/rez/blob/master/src/rez/system.py#L78

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).

@bpabel
Copy link
Contributor Author

bpabel commented Nov 22, 2017

What is the status of this? I pulled down the latest master, which looks like it has #341 merged (except looking at the #341 now, the changes look different than I remember). Either way, it looks like rez is still not dropping into an interactive shell on windows.

@instinct-vfx
Copy link
Contributor

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.

@bpabel
Copy link
Contributor Author

bpabel commented Nov 22, 2017

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 would love to see the different overlapping PRs for windows fixed once and for all.

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.

@instinct-vfx
Copy link
Contributor

Yes, #460 is ready to be merged. I made the changes according to the discussion with Alan in #459

I don't have write access though, so i can not merge myself.

@@ -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"
Copy link
Contributor Author

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.

@bpabel
Copy link
Contributor Author

bpabel commented Nov 28, 2017

Includes the changes from #460 / #459 and removes sh and pbs dependencies.

@nerdvegas
Copy link
Contributor

Righto, I'm on this today.

@nerdvegas nerdvegas mentioned this pull request Nov 29, 2017
@nerdvegas nerdvegas merged commit efb1c3d into AcademySoftwareFoundation:master Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants