Better Windows support #103
Conversation
Makefile
Outdated
| @@ -35,32 +35,39 @@ install-html: | |||
| lint: lint-black lint-isort lint-flake8 lint-t | |||
|
|
|||
| lint-black: | |||
| $(PYTHON) -m black --check --quiet --diff . stg | |||
| $(PYTHON) -m black --check --quiet --diff . stgit/main.py | |||
There was a problem hiding this comment.
We do not need to add stgit/main.py to the command line of black, isort, or flake8. Those tools already find all the appropriate .py files, including stgit/main.py. The reason the stg script was here explicitly was because it lacked a .py extension. So passing . is sufficient for all three tools.
stgit/utils.py
Outdated
| @@ -93,7 +95,12 @@ def hook(*parameters): | |||
|
|
|||
| # On Windows, run the hook using "bash" explicitly | |||
| if os.name != 'posix': | |||
There was a problem hiding this comment.
Since the new behavior is even more Windows-specific, perhaps this test should become stronger. I.e. os.name == 'nt' or platform.system() == 'Windows'.
We may also wan to preserve the old behavior for non-Windows non-posix platforms. I.e. something like:
if os.name == 'nt':
... # new behavior
elif os.name != 'posix':
... # old behavior
There was a problem hiding this comment.
True, but I will have to make sure that I don't break Cygwin. There's some weirdness there, if I remember correctly, in that it identifies both as Windows and POSIX.
stgit/utils.py
Outdated
| git_exe = shutil.which('git.exe') | ||
| if not git_exe: | ||
| raise StgException('Failed to locate git.exe') | ||
| bash_exe = pathlib.Path(git_exe).parents[1] / 'bin' / 'bash.exe' |
There was a problem hiding this comment.
Something I'm finding troubling here is that we seem to be subverting the user's PATH with this approach. I.e. if the user has their PATH setup such that there is a bash.exe available in a different location than their git.exe, we would override their PATH by forcing git's bash.exe to be used.
Along these lines, it's not entirely clear to me why the solution to this problem isn't to require the user to have a properly constructed PATH?
Also, in what circumstances is git.exe available, but bash.exe is not? StGit doesn't do anything special on Windows to find git.exe; so if bash.exe is in the same directory as git.exe, why do we have to perform this dance to find bash.exe relative to git.exe?
The comment seems to indicate that bash.exe is sometimes incorrectly found in the `Windows/System32' directory. Under what circumstances does this happen? I'm not a regular user of Windows, so I need help connecting these dots.
There was a problem hiding this comment.
The problem is that C:\Windows\System32\bash.exe is just a stupid bootstrapper that allows the user to install WSL, and it is present on all recent versions of Windows. Completely and utterly useless. For me the problem was that the bin directory of Git for Windows was appended to the PATH, i.e. after C:\Windows\System32. Hence, git.exe is found, but bash.exe resolves to this shim.
But I agree, subverting the user's PATH is bad. I'll have a look at how Git for Windows deals with this.
Makefile
Outdated
| .install: build | ||
| pyver=`$(PYTHON) -c 'import sys; print(".".join(map(str, sys.version_info[:2])))'` && \ | ||
| prefix="$(PWD)/inst/python$$pyver" && \ | ||
| sitepackages="$$prefix/lib/python$$pyver/site-packages" && \ | ||
| mkdir -p "$$sitepackages" && \ | ||
| PYTHONPATH="$$sitepackages" $(PYTHON) setup.py develop --prefix="$$prefix" |
There was a problem hiding this comment.
I'm not immediately comfortable with this approach. It's both different than StGit's existing non-setuptool strategy as well as being unconventional relative to how setuptools and entry-points are normally used.
In other setuptools projects that I work on, the pattern is to run pip install -e or setup.py develop to reify the entry points into runnable programs. This typically happens in the context of a virtualenv, but could also conceivably be done in the context of the per-user site-packages (PEP 370).
Creating this inst directory instead of either using a virtualenv or the user's site-packages is the part that seems unconventional to me--and perhaps unnecessarily so?
I'm a little fuzzy on whether setuptools is truly needed on Windows or whether it is a convenience versus the status quo? Is it not possible to install StGit correctly on Windows without this change?
There was a problem hiding this comment.
The problem on Windows is that you need the exe-wrapper.
The reason I added the .install target was that the tests need a runnable version of stgit. Maybe the .install target is not necessary at all and make test can be run in a simpler way? I'll investigate a bit more.
t/test-lib.sh
Outdated
| PYTHON="${PYTHON:-python}" | ||
| python_major_minor="$($PYTHON -c ' | ||
| import sys | ||
| print(".".join(map(str, sys.version_info[:2])))')" | ||
| stg_bin_dir="$stg_build_dir"/scripts-"$python_major_minor" | ||
| stg_inst_dir="$STG_ROOT/inst/python$python_major_minor" | ||
| stg_bin_dir="$stg_inst_dir"/bin # TODO on Windows this is .../Scripts |
There was a problem hiding this comment.
This comment sort of implies that the test suite may not actually be runnable on Windows. What's the status of that?
There was a problem hiding this comment.
Windows is... difficult :-) Honestly, I forgot about that before making the PR. The whole thing's been sitting on my computer for far too long.
|
Thank you for this PR @wildmichael. It's good to know that there is someone out there using StGit on Windows. There is a lot going on in these patches and I have several comments and questions that I've added as review comments. |
wildmichael
force-pushed
the
feature/windows-support
branch
from
d268509 to
b70819c
Compare
|
@jpgrayson I rebased to your current
|
Looks like one more instance of this remains in the
My feeling is that you're on the right track with this approach. But given that the CI failed, it seems this may need some more attention.
I appreciate the explanation you provided about the Windows-provided bash.exe found in the System32 directory. This updated approach seems okay. And if it's not perfectly okay, it is at least well-contained to Windows platforms. Thanks for the additional forward progress on this PR. Obviously need to resolve the failing CI as a key next step. I'm also wondering if we might want to split this into a couple of pieces. For example, the PEP-440 version strings is largely orthogonal and non-controversial, so we could get that piece merged sooner than later if it was in its own PR. |
wildmichael
force-pushed
the
feature/windows-support
branch
from
b70819c to
3aa91d5
Compare
|
Finally the CI runs through cleanly on my fork (embarrassing, sorry). I also found a way of getting around the While tinkering I found that some tests that grep the output of |
wildmichael
force-pushed
the
feature/windows-support
branch
from
43acf68 to
9eaaaa2
Compare
| #!/bin/sh | ||
|
|
||
| # This script is only used by the tests so they can work without stgit being | ||
| # installed. It assumes PYTHONPATH to be set up correctly. |
There was a problem hiding this comment.
I think we want to avoid making changes like this to the test infra. The test infra is heavily borrowed from git -- every change we make makes it more difficult to update our infra to match git's. If think is something we think is important the best place to add it might be the git repo itself.
Is this something that's needed for Windows?
There was a problem hiding this comment.
Sorry for being MIA for so long... Yes, I agree, it is bad. Maybe move it out of t/? As to your question: no, it is more related to setuptool packaging.
| done | ||
|
|
||
| .PHONY: format test test-patches | ||
| .PHONY: format test test-patches .install |
There was a problem hiding this comment.
I think the rule has been removed?
| @@ -71,10 +71,11 @@ coverage-test: | |||
| $(MAKE) .coverage | |||
|
|
|||
| .coverage: | |||
| rm -rf build | |||
| rm -rf install | |||
There was a problem hiding this comment.
what is adding this install file? am i missing something?
| -mkdir .cov-files | ||
| COVERAGE_FILE=$(CURDIR)/.cov-files/.coverage \ | ||
| $(PYTHON) -m coverage run --context=setup setup.py build | ||
| $(PYTHON) -m coverage run --context=setup setup.py install |
| @@ -96,6 +97,7 @@ clean: | |||
| $(MAKE) -C $$dir clean; \ | |||
| done | |||
| rm -rf build | |||
| rm -rf inst | |||
When running hooks on Windows, bash.exe must be supplied on the command line to run the hook script. This change tries to find a viable bash.exe from the user's PATH, while avoiding the bash.exe stub in the OS's System32 directory. This change is adapted from Michael Wild's PR #103. Signed-off-by: Peter Grayson <[email protected]>
|
I've posted a discussion topic #152 that is directly related to this PR. @wildmichael @topher200 |
This allows stgit to work seamlessly on Windows where setuptools automatically creates a wrapper executable. On other platforms, a wrapper script is generated. Requires setuptools instead of distutils. The little functionality that was present in stg has been migrated to stg.main:main(). Signed-off-by: Michael Wild <[email protected]>
Signed-off-by: Michael Wild <[email protected]>
Make t/stg executable
Don't output current working directory to stdout in t/stg
Simplify by removing stgit/__main__.py and directly calling stgit.main as module from t/stg
Remove more instances of stgit/main.py being passed to linting tools.
wildmichael
force-pushed
the
feature/windows-support
branch
from
9eaaaa2 to
37b43b4
Compare
|
I've pushed several changes that overlap with the changes and goals of this PR:
So at this point, I think all of the goals of this PR are accomplished. Would love feedback from @wildmichael or any other Windows users out. Would like to close this PR if everything is covered. |
|
So far your I think the message about PEP 517 is a fluke, the real error is |
|
Thanks @wildmichael for taking the time to try this out on Windows--I really appreciate it.
That was not a use case I had considered.
I think I know the problem. And you're right that it is all about the generated files, which includes When building from a local git worktree,
The import-ability of I'll note that in the context of either an sdist tarball, a git archive tarball, or a wheel (.whl), the generated files are included in those distributions such that the setup.py-time code generation is unnecessary. So if one were to attempt to install StGit from any of those, e.g. with Anyway, I do think it would be good for the use case of installing from a git URL to work, so I'll try to figure that out. |
|
Once released on pypi or as a conda package, this indeed shouldn't be an issue for most users anymore. For packages where I want to try out a specific branch though, I often pip install from a git URL, and I don't think I'm the only one. If it proves too hard or hacky to work around this problem, maybe there is a way of giving a better error message? Having to clone to /temp and then pip install really isn't that much of a problem once you realize what the issue is. |
|
I was able to update I'm closing this PR. Let's open new issues for any further problems we find with Windows, packaging, or installation. |
|
I just gave it a shot, and can confirm it works like a charm! Thank you so much. No I can start proselytizing stgit to my work mates 😀 |
This branch provides a few fixes / changes to enable better support for Windows (using wrapper executables). Further, pip installation works better because
git_describe_version()creates PEP 440 compliant version numbers.