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

Windows python support should be made easier #23083

Closed
TheGoddessInari opened this issue Mar 23, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@TheGoddessInari
Copy link
Contributor

commented Mar 23, 2019

On my systems (as many are), I have both Python 2 and 3 installed with 3 being the default.

I personally symlink python 2.x to python2 and python2.7 on the PATH, but the Windows python versions provide its own solution to this: py -2, which is installed by default.

Even manually running mach via python 2.7 runs into quite a few blockers, including the
''':' && if [ ! -z "$MSYSTEM" ] ; then exec python "$0" "$@" ; else which python2.7 > /dev/null 2> /dev/null && exec python2.7 "$0" "$@" || exec python "$0" "$@" ; fi
block at the top of mach getting very confused and not allowing execution, and python/mach_bootstrap.py trying to explicitly override your python choice (again!), when it could just run the equivalent of python -m pip and python -m virtualenv from whatever version of python you're trying to run it with already.

In the case of python/mach_bootstrap.py, for instance, this results in a Python 3.x virtualenv if python 3.x is the default on my PATH, even running mach via python 2.x and having python2 and python2.7 binaries resolvable on the PATH.

For whatever reason mach is still using Python 2, it should better support the fact that mach is already being run from a python interpreter and shouldn't need to try to make guesses on what the user's path/environment is and override the execution.

py -2 should probably also be the default from mach.bat.

I nearly gave up on Servo because of the above problems because it wasn't clear what was going wrong, especially with the python/mach_bootstrap.py issue.

@tigercosmos

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2019

There’s room for improvement. We would be glad if you can help improve the code.
Do you finally build the servo successfully? I know it is always hard to set up the environment and build on Windows.

@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

I can certainly look into it soon. Currently it doesn't get through all of the dependencies for me because cmake-rs needs updating for VS2019, but at least mach is working as expected with workarounds.

I have some other ideas on how to (possibly) make Windows support easier (that I could presumably also work on), but that depends on some other infrastructure updates anyway.

How opposed would servo be on optionally seeing (via cmake) if vcpkg is installed and working on Windows before dragging in prebuilt binaries for end users? Or would that add too much non-determinism for the build process?

vcpkg lets Windows people add and manage compiled packages for other things to include in their build process. Right now, I think the main thing missing from vcpkg is gstreamer, which I can always try to get included. I think the downside with vcpkg is that there's no standard path on which it'd be installed or detectable.

@tigercosmos

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2019

I conservatively thought that the Servo team might not want to change the infra of building, since it would take a risk. However, I encourage you to talk to the members of the team on IRC if you really want to do something cool.

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I would be interested in learning more about how vcpkg could make building on Windows easier.

@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

vcpkg represents a development-minded package manager from Microsoft on that builds many things from source. It's pretty easy to use and extensible. It makes the most sense on Windows, at least if I can help them make sure they're discoverable from cmake. Since it's from Microsoft and open source, it's about as official as it gets for the platform.

It wouldn't necessarily make building easier in all cases, but would make it easier to keep things consistent. If someone's set up a development environment on their machine, presumably it'd be useful to ensure that the same MSVC version is used for all C, C++, and Rust bits and everything is ABI and linker compatible.

Of course, none of it applies (much) to MSYS2 or other things that are sufficiently UNIX-y.

I'd noticed servo downloading binaries for things like OpenSSL, which vcpkg can provide. Just as an optional thing to be able to pull library and header availability from would be nice. If I'm compiling things with cargo with the MSVC ABI, I'm already using cl.exe under the hood and it can be a good idea to make as much as possible compiled in the same fashion.

As I said, I think the only major piece missing that servo uses is gstreamer, but I can always file pull requests for that too.

@TheGoddessInari TheGoddessInari referenced this issue Mar 26, 2019

Merged

Py2 #23098

4 of 5 tasks complete
@TheGoddessInari

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I just realized as for vcpkg, there's a vcpkg crate which entirely simplifies things, and one of servo's dependencies already uses.

I'll definitely try to get gstreamer on there regardless as it could be useful.

As a sidenote, Rust infrastructure seems to be a lot more responsive than anything node-related. That's very reassuring to someone who's been run ragged trying to get bugs fixed elsewhere for ages.

bors-servo added a commit that referenced this issue 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 -->

bors-servo added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 -->
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.