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

Need to decode repo_working_dir before calling CreateProcess() #4

Closed
rmanibus opened this issue Oct 14, 2019 · 5 comments
Closed

Need to decode repo_working_dir before calling CreateProcess() #4

rmanibus opened this issue Oct 14, 2019 · 5 comments
Labels
bug Something isn't working Windows

Comments

@rmanibus
Copy link

Hello,

I had an error on windows 10 :

TypeError: CreateProcess() argument 8 must be str or None, not Byte

I solved the problem by decoding the repo_working_dir line 1437.

@newren
Copy link
Owner

newren commented Oct 15, 2019

Oh, cool, my first Windows user! I don't have Windows to test on, so this is great feedback. Was this the only change you needed?

It looks like I've got a little bit of inconsistency in how source and target directories are handled, with either potentially ending up as a cwd argument to subprocess.Popen. I suspect there are a few more that would be needed depending on the arguments you pass; I should probably go through and clean those up. But I believe I also mixed and matched regular strings and byte strings in command arguments; do those all work for you on windows despite it not liking a cwd that was a bytestring? E.g. does

subprocess.check_output(["git", b"--version"])

work?

@newren newren added bug Something isn't working Windows labels Oct 15, 2019
@rmanibus
Copy link
Author

rmanibus commented Oct 15, 2019

No sorry I created this a bit too fast.

Each time you call check_output I replaced the argument by
repo_working_dir.decode('utf-8') if isinstance(repo_working_dir, bytes) else repo_working_dir

@newren
Copy link
Owner

newren commented Oct 21, 2019

I created a branch called 'windows-workarounds' to address this issue, but I suspect it might just be the first issue that needs to be fixed on Windows and that there will be others you'll run into. Any chance you could try that branch and report what the next error you hit is?

@rmanibus
Copy link
Author

Thank you, it's working perfectly (I tried --path and --path-rename).

The only thing is that by default the python binary is called python and not python3.

@newren
Copy link
Owner

newren commented Oct 22, 2019

Wow, that's all that was necessary to get it to work on Windows? I'm a bit surprised, but I'll go ahead and merge that commit in. If you do run into further issues, go ahead and report them.

As far as "python" vs "python3" -- yeah, I kind of expect that. For years the python devs gave the advice that "python" should refer to python2, and the executable for python3 should be named differently. They are starting to relax that with the impending EOL of python2, which means we'll be stuck with some systems calling it "python" and some calling it "python3". However, it's safer for me to use "python3". If someone has "python" which is a python3 implementation, then they probably get a simple error message saying there is no "python3" executable. In contrast, if I were to use "python" assuming that meant python3, then those who have a "python3" will probably also have a "python" that refers to python2 -- and the error messages from attempting to run python3 code under python2 will be far less clear. So, because of that, it'll be a long time before I switch over the shebang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Windows
Projects
None yet
Development

No branches or pull requests

2 participants