Fix broken tag prefix on Windows and add CI #283
Conversation
For `pip` the problem in Windows was that `pip install` didn't have the correct permissions to the pip uninstall file. Using `python -m pip install` alleviates this access issue. For `rundemo`: for some of the demo projects, this is simply a python file. In Linux you could make it runnable on its own through a shebang line (`#!/usr/bin/env python`) but this doesn't work on windows CMD or Powershell. In these cases we run `python rundemo` explicitly instead, controllable through the `use_python` argument.
…po` test case. Closes python-versioneer#282
From Python=3.7 onwards, Windows has trouble recognizing virtual environments made by virtualenv<20 because they don't contain a pyvenv.cfg file. It may be related to the issue mentioned here: https://bugs.python.org/issue35872 The pyvenv.cfg files _are_ generated by virtualenv>=20, which resolves the problem.
|
Thank you! I will try to review this ASAP, but please feel free to bug me in a day or two if I haven't gotten to it. |
There was a problem hiding this comment.
Looks great! Minor comments.
There was a problem hiding this comment.
Addressing your question about the explosion in jobs/runtime, this proposes only testing Windows on one Python version, but three shells.
I'm not sure how well tox will work with these other shells, so this might not be the greatest idea. But it feels a bit cleaner than tacking on extra tests for Windows.
|
@mathijsvdv All my suggestions were implemented and debugged in https://github.com/python-versioneer/python-versioneer/tree/maint/windows. If you're good with that, please merge it into your branch. |
|
@effigies Sounds good to me! I had a quick look at your adjustments and they generally make sense to me. I'll see how the tests are working now and let you know my feedback in the next few days. |
I agree. One reason I didn't run the whole tox suite was that I was running into some errors when running the whole thing on other shells (and test_git.py contained the tests for the tag prefix). But with your version it seems to be working fine. Testing fewer Python versions seems a fine compromise to me. One comment I'd make: there was a notable change starting from Python 3.7 that caused the virtual environments to no longer work on Windows until I updated |
|
Python 3.6 is end of life. I'd suggest we drop that altogether, but we could do 3.7 to test the range. |
Fair enough. Since the issue with virtualenvs started with Python 3.7 I think we're pretty covered with Python 3.10 + all the linux tests then, but it can't hurt to cover our bases. I'll leave it to you. For the rest I have no comments. From my side, we're ready to merge :) |
|
Let's do it, and we can tweak later if needed. Thanks a ton for pushing this forward! |
|
Great work! Thanks for the fix guys. Some quick tests seem to confirm that this fixes the issue for me on Windows 10 and Python 3.7. |
|
No problem, happy to help! |
|
Any chance this will released soon? Current release is broken for some cases when running windows. |
|
Yes. I would like to get in #280 before releasing, if anybody has the urge to do a review... |
This pull request fixes #273 by simply removing the
--matchargument whentag_prefix = ''. No platform-dependent escaping of the asterisk needed.As a first verification I tracked what
git describe --tags --match <Match string>produces for each platform (Windows/Linux) and shell (cmd/powershell/bash), given different arguments for--match. In the table below we have a repository with two tags:aaa-999anddemo-1.0.--match>The above suggests that we can indeed cover all cases by:
--matchwhentag_prefix = ''--match {tag_prefix}*otherwiseNext, I added the
test_no_tag_prefixunittest to cover the case wheretag_prefixis empty.This covers the fix, but most of my efforts have actually been to get the Windows tests working to be sure that the implementation is correct. I started from #263 and added the following changes:
os.path.joinwherever possible rather than hardcoding forward-slash paths 570fcc8rundemofile is a Python file that is made runnable in Linux through a shebang line. This doesn't work in Windows, so I opted to make apython rundemocall in these cases instead.pip install -U pipdoesn't seem to work in Windows because of access issues. You need to usepython -m pip install -U pipinstead, so I changed this in therun_in_venvfunction.virtualenv<20is not recognized by Windows inpython>=3.7because it misses apyvenv.cfgfile. Therefore I switched tovirtualenv>=20and made the necessary changes to the testing code.A couple of attention points, open to suggestions here:
test_distandtest_dist_subprojectfailing #282,distutilsare deprecated and thetest_sdistandtest_sdist_subprojectwere already failing inmaster, so I took the liberty of removing these two unittests.test_gitscript from bash, CMD and powershell when on Windows to cover the bases in the table above. Perhaps it's worth reducing the number of Python versions checked for Windows or to remove one of those extra scripts (CMD and Powershell seem to behave identically here).