These changes do not require tests because it changes the python bootstrap slightly and the changes are obvious.

The virtualenv changes shouldn't disrupt anything as it just uses the existing python (being used) to call itself -m virtualenv, and only if it exists. imp is still apparently the preferred builtin way to find this in Python 2.x without actually importing it. Importing it would cause an unused import warning in tidy. It still picks up the new things in _virtualenv, just no longer has a special case for Win32/MSYS because it's no longer needed.

The .bat change is the simplest I could think of that allows fallback in both cases and is no worse than before. where /Q is documented by Microsoft to return 0 if a successful match is found, and print nothing in either case.

This comment has been minimized.

As I'm getting further and further into bootstrapping Servo, it looks like mach.bat needs to be reworked to support Visual Studio 2019, as well as not screw up the environment if one is already provided (whether VS Command Line, or manually provided).

Otherwise, it's simply too aggressive and clobbers the environment which causes even further quiet errors that are confusing to resolve.

This comment has been minimized.

Failing any syntax errors, that should support both VS2019 and user-supplied environments from mach.bat. Bases an existing environment off of %VCINSTALLDIR%, same as cmake-rs.

There is technically a semantics change with using setlocal/endlocal in mach.bat, but it keeps the environment clean so is probably a net win. Would reinitialize the Visual Studio environment (if one isn't supplied) each time mach.bat is called, but also makes abundantly clear which compiler is being used.

Py2
<!-- Please describe your changes on the following line: -->
86bb79e: Rework mach.bat to support VS2019 and user-supplied environments.
f6bd2d2: Default mach.bat to using py -2.
81d48c9: Don't assume the user's environment in mach_bootstrap.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#23083 (GitHub issue number if applicable)
<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it changes the python bootstrap slightly and the changes are obvious.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
The virtualenv changes shouldn't disrupt anything as it just uses the existing python (being used) to call itself `-m virtualenv`, and only if it exists. imp is still apparently the preferred builtin way to find this in Python 2.x without actually importing it. Importing it would cause an unused import warning in tidy. It still picks up the new things in _virtualenv, just no longer has a special case for Win32/MSYS because it's no longer needed.
The .bat change is the simplest I could think of that allows fallback in both cases and is no worse than before. `where /Q` is documented by Microsoft to return 0 if a successful match is found, and print nothing in either case.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23098)
<!-- Reviewable:end -->

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

On Windows with multiple Pythons installed, this was causing python2.7
to bootstrap a 3.7 virtualenv that it couldn't make use of.
PIP_NAMES wasn't used at all, and VIRTUALENV_NAMES ends up being unused
now.

Servo already assumes the user has Python, this is the primary
way to make sure that Python 2 is preferred, and you should
get a sensible error message if you have python 3 but not 2.
But first, check that py.exe exists because if a system has only
Python 2.x only, it won't have it. If it doesn't, then try python.exe.

edited

Got confused by the github interface.

All that it should need is the virtualenv package being in the same place as whatever python interpreter is being used for mach in the first place.

The previous behavior was calling from the PATH, which can end up a correctness issue and is why I was silently getting python 3.7 packages on Windows in the virtualenv in the first place. It could call a different python and different virtualenv than what it was called with to begin with.

Removing sudo on the Travis CI should work, but Travis CI is already inside of a virtualenv to begin with.

Py2
<!-- Please describe your changes on the following line: -->
da31023: Rework mach.bat to support VS2019 and user-supplied environments.
4551f60: Default mach.bat to using py -2.
03e4708: Don't assume the user's environment in mach_bootstrap.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#23083 (GitHub issue number if applicable)
<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it changes the python bootstrap slightly and the changes are obvious.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
The virtualenv changes shouldn't disrupt anything as it just uses the existing python (being used) to call itself `-m virtualenv`, and only if it exists. imp is still apparently the preferred builtin way to find this in Python 2.x without actually importing it. Importing it would cause an unused import warning in tidy. It still picks up the new things in _virtualenv, just no longer has a special case for Win32/MSYS because it's no longer needed.
The .bat change is the simplest I could think of that allows fallback in both cases and is no worse than before. `where /Q` is documented by Microsoft to return 0 if a successful match is found, and print nothing in either case.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23098)
<!-- Reviewable:end -->

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

Py2
<!-- Please describe your changes on the following line: -->
da31023: Rework mach.bat to support VS2019 and user-supplied environments.
4551f60: Default mach.bat to using py -2.
03e4708: Don't assume the user's environment in mach_bootstrap.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#23083 (GitHub issue number if applicable)
<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it changes the python bootstrap slightly and the changes are obvious.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
The virtualenv changes shouldn't disrupt anything as it just uses the existing python (being used) to call itself `-m virtualenv`, and only if it exists. imp is still apparently the preferred builtin way to find this in Python 2.x without actually importing it. Importing it would cause an unused import warning in tidy. It still picks up the new things in _virtualenv, just no longer has a special case for Win32/MSYS because it's no longer needed.
The .bat change is the simplest I could think of that allows fallback in both cases and is no worse than before. `where /Q` is documented by Microsoft to return 0 if a successful match is found, and print nothing in either case.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23098)
<!-- Reviewable:end -->

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

This comment has been minimized.

Py2
<!-- Please describe your changes on the following line: -->
da31023: Rework mach.bat to support VS2019 and user-supplied environments.
4551f60: Default mach.bat to using py -2.
03e4708: Don't assume the user's environment in mach_bootstrap.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#23083 (GitHub issue number if applicable)
<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because it changes the python bootstrap slightly and the changes are obvious.
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
The virtualenv changes shouldn't disrupt anything as it just uses the existing python (being used) to call itself `-m virtualenv`, and only if it exists. imp is still apparently the preferred builtin way to find this in Python 2.x without actually importing it. Importing it would cause an unused import warning in tidy. It still picks up the new things in _virtualenv, just no longer has a special case for Win32/MSYS because it's no longer needed.
The .bat change is the simplest I could think of that allows fallback in both cases and is no worse than before. `where /Q` is documented by Microsoft to return 0 if a successful match is found, and print nothing in either case.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23098)
<!-- Reviewable:end -->

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.