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

Py2 #23098

Merged
merged 4 commits into from Apr 2, 2019

Conversation

Projects
None yet
5 participants
@TheGoddessInari
Copy link
Contributor

commented Mar 26, 2019

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.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23083 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • 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 change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Mar 26, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Mar 26, 2019

Heads up! This PR modifies the following files:

@TheGoddessInari TheGoddessInari force-pushed the TheGoddessInari:py2 branch from a5fe296 to fe2eddb Mar 26, 2019

@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I can't quite figure out what's up with the automated Trusty failures.

I've tried on minimal 16.04 and 18.04 VMs with python-virtualenv (which I believe both buildbots say they have) and it works fine.

I made a revision to match the same style with minimal changes and not require virtualenv 'early'.

@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

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.

@TheGoddessInari TheGoddessInari force-pushed the TheGoddessInari:py2 branch 2 times, most recently from 61a6b53 to 86bb79e Mar 26, 2019

@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

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.

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

These changes break using mach on linux:

$ ./mach test-tidy --no-progress --all
Python virtualenv is not installed. Please install it prior to running mach.
The command "./mach test-tidy --no-progress --all" exited with 1.
@jdm
Copy link
Member

left a comment

I'm fine with the goal of these changes, but I don't know enough about the low-level details of imports and modules and different versions to suggest how not to break our CI, unfortunately.

Show resolved Hide resolved python/mach_bootstrap.py Outdated
Show resolved Hide resolved python/mach_bootstrap.py

@jdm jdm assigned jdm and unassigned Manishearth Mar 29, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

⌛️ Trying commit 86bb79e with merge 95f2587...

bors-servo added a commit that referenced this pull request Mar 30, 2019

Auto merge of #23098 - TheGoddessInari:py2, r=<try>
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 -->
@jdm

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Presumably we will need:

  • this to change to follow this instead
  • this to run a pip install instead

Depending on the results of the try run, we may need to adjust https://github.com/servo/saltfs/ for the rest of our CI as well.

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Windows taskcluster:

C:\Users\task_1553915082\repo>mach fetch 
Visual Studio 2015, 2017, or 2019 is not installed.

@TheGoddessInari TheGoddessInari force-pushed the TheGoddessInari:py2 branch from 86bb79e to da31023 Mar 30, 2019

@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Cleaned up the mach issue (I forgot about the environment), as well as the requested change on mach_bootstrap.py.

TheGoddessInari added some commits Mar 26, 2019

Don't assume the user's environment in mach_bootstrap.
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.
Default mach.bat to using py -2.
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.
Rework mach.bat to support VS2019 and user-supplied environments.
As a bonus, use setlocal to avoid environment pollution.

@jdm jdm force-pushed the TheGoddessInari:py2 branch from 06a70d3 to 09bc83f Apr 1, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Hmm, even sudo pip install virtualenv on travisci is not making the module import check pass :<

@TheGoddessInari
Copy link
Contributor Author

left a comment

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.

Show resolved Hide resolved .travis.yml Outdated
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

⌛️ Trying commit dee8803 with merge 1760da1...

bors-servo added a commit that referenced this pull request Apr 1, 2019

Auto merge of #23098 - TheGoddessInari:py2, r=<try>
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 -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

💔 Test failed - mac-rel-wpt2

@jdm jdm force-pushed the TheGoddessInari:py2 branch from dee8803 to c0052c0 Apr 1, 2019

@highfive highfive removed the S-tests-failed label Apr 1, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

📌 Commit c0052c0 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

⌛️ Testing commit c0052c0 with merge 01fc4a0...

bors-servo added a commit that referenced this pull request Apr 1, 2019

Auto merge of #23098 - TheGoddessInari:py2, r=jdm
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 -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

bors-servo added a commit that referenced this pull request Apr 1, 2019

Auto merge of #23098 - TheGoddessInari:py2, r=jdm
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 -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

⌛️ Testing commit c0052c0 with merge 3340214...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

@bors-servo bors-servo merged commit c0052c0 into servo:master Apr 2, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@jdm jdm referenced this pull request Apr 16, 2019

Open

Add virtualenv installation to build documentation. #23212

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.