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

Add CI for Windows #1158

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

chrisjbremner
Copy link
Contributor

As discussed in #1157, in order for pex to officially support Windows, we need to add Windows CI. I did a quick survey of other OSS Python projects that have some form of Windows CI, and there wasn't a clear favorite (there also just weren't many projects using Windows CI). I figured that since Travis supports Windows and this project already uses Travis, it would make sense to try to leverage the existing infrastructure. Using this Github search, I came across this travis.yml that sets up a Windows CI pipeline for testing a Python project. I adapted that format to fit into the existing infrastructure of this project.

While I don't expect this to work on the first try, I think this should be a good start to allow us to figure out what adjustments need to be made.

@Eric-Arellano
Copy link
Contributor

Exciting! Depending on how many failures there are, we may want to use pytest.mark.skipif for any tests that fail on Windows. That way we can land a big checkpoint and fix the rest in followups. This is how we did Pants's Python 2 -> 3 migration.

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a swing at this!

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@jsirois
Copy link
Member

jsirois commented Dec 31, 2020

@chrisjbremner you might want to kill all the non-Windows shards in .travis.yml to speed up iteration. We share a queue with pantsbuild/pants which can sometimes slow things down a good bit not to mention there are a lot of shards in this CI setup that run before the ones you've added.

@chrisjbremner
Copy link
Contributor Author

By "kill" do you mean temporarily delete them/comment them out?

@jsirois
Copy link
Member

jsirois commented Dec 31, 2020

By "kill" do you mean temporarily delete them/comment them out?

Yes.

.travis.yml Outdated
- choco install python --version 3.9.0
- python -m pip install --upgrade pip
- choco install python --version 3.9.1
- python -m ensurepip --upgrade
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not the python you think it is. CI says:

python2 v2.7.18 [Approved]

python2 package files install completed. Performing other installation steps.

Installing 64-bit python2...

python2 has been installed.

Installed to 'C:\Python27'

Adding C:\Python27 to PATH if needed
...
$ python -m pip install --upgrade pip

The command "python -m pip install --upgrade pip" failed and exited with 1 during .

Your build has been stopped.

C:/ProgramData/chocolatey/lib/mingw/tools/install/mingw64/opt/bin/python.exe: No module named pip

Note chocolatey C:\Python27 vs C:/ProgramData/chocolatey/lib/mingw.... FWICT the latter is the "A basic Python 2.7.9 interpreter is also included: /C/ProgramData/chocolatey/bin/python.exe" referred to at the bottom of this section in the docs: https://docs.travis-ci.com/user/reference/windows/#pre-installed-chocolatey-packages

@chrisjbremner
Copy link
Contributor Author

Ok, looks like it is working now and the Windows tests are failing with the fcntl issue fixed in #1157. I guess I'll merge that branch into this one and change the target of this PR to that branch?

@Eric-Arellano
Copy link
Contributor

I think it'd be best to skip those failing tests in this PR, then you can unskip them in the fix. We want to keep these separate for cleaner diffs.

Thanks for spearheading this!

@jsirois
Copy link
Member

jsirois commented Dec 31, 2020

I disagree with Eric on this one and would prefer a real green all in one here with the .travis.yml and the code fix.

@chrisjbremner
Copy link
Contributor Author

Ok, it will probably take a while to fix all of these tests

@jsirois
Copy link
Member

jsirois commented Jan 10, 2021

@chrisjbremner one thing you might try is not fighting so hard against the motivating example tox.ini you found. That tox.ini sets PATH: https://github.com/mapaction/mapactionpy_controller_dependencies/blob/6b3fcce09d29c20d6ec51040fd0449f0ab3d984a/.travis.yml#L12

Perhaps do things just like they do.

@g-cassie
Copy link

g-cassie commented Mar 3, 2021

@chrisjbremner just wondering how close you were on this. It's a blocker for us now too so we could pick it up and see if we can take it the rest of the way if you don't have the bandwidth.

@jsirois
Copy link
Member

jsirois commented Mar 3, 2021

If you do pick it up @g-cassie - which would be great - note that Pex has since converted from Travis-CI to Github Actions. They still have windows support so you should have a clear path to getting Windows CI setup still.

@chrisjbremner
Copy link
Contributor Author

Hi @g-cassie, I haven't got much farther than where this is left, I've been a bit distracted by other things recently. If you could help out, I'd appreciate it.

@jacobnlsn
Copy link

@chrisjbremner @g-cassie @jsirois I am picking this up today and planning on working through the required CI changes for windows via the GitHub Actions. I am going to create a fresh fork and manually copy over the changes to pex/common.py from this PR and will credit @chrisjbremner and link to this PR in my PR. Let me know if there is a preferred way.

@stuhood
Copy link

stuhood commented Mar 9, 2021

@chrisjbremner : @jacobnlsn is taking a swing at this, but would welcome your insight. If you're interested, joining the #pex channel in pantsbuild slack would be really helpful!

Base automatically changed from master to main March 18, 2021 21:23
@mshafer-NI
Copy link

@jacobnlsn, were you able to make any progress on this?

@jacobnlsn
Copy link

@jacobnlsn, were you able to make any progress on this?

No I was not unfortunately.

@mshafer-NI
Copy link

| @jacobnlsn, were you able to make any progress on this?

No I was not unfortunately.

Ok, thank you for the quick response. I'm investigating picking this up.

@mshafer-NI
Copy link

Well, my investigation for the day has ended with "the core functionality and enabling the CI is doable, but now there's lots of assuming Posix in the tests"... -> https://github.com/mshafer-NI/pex/commits/add_windows_ci

@benjyw
Copy link
Collaborator

benjyw commented May 31, 2022

Hi all, checking in on the status of this... E.g., has there been an attempt to get Pex to run manually on a simple example, before trying to get all the tests to pass?

@mshafer-NI
Copy link

Hi all, checking in on the status of this... E.g., has there been an attempt to get Pex to run manually on a simple example, before trying to get all the tests to pass?

granted, I went straight to the tests: https://github.com/mshafer-NI/pex/actions/runs/2174174654

@benjyw
Copy link
Collaborator

benjyw commented May 31, 2022

granted, I went straight to the tests: https://github.com/mshafer-NI/pex/actions/runs/2174174654

Bold :)

It might make sense to first try and get something really simple like

$ python -m pex ansicolor -o dummy.pex

working manually?

@mshafer-NI
Copy link

It might make sense to first try and get something really simple like
$ python -m pex ansicolor -o dummy.pex

whelp, looks like there are more places to get the /bin assumption fixed in my branch:

"c:\Program Files\Python38\python.exe" -m pex ansicolor -o dummy.pex
Failed to spawn a job for C:\Program Files\Python38\python.exe: The virtualenv at C:\Users\mshafer\.pex\venvs\7a1c9fddd44690726e1ace55698aa05bf341787d\1be1ff869bb27ae7c6706e26813ff061a3ee4ea1.5c554b2f67064776bfcb5d4225b30bae is not valid. Failed to load an interpreter at C:\Users\mshafer\.pex\venvs\7a1c9fddd44690726e1ace55698aa05bf341787d\1be1ff869bb27ae7c6706e26813ff061a3ee4ea1.5c554b2f67064776bfcb5d4225b30bae\bin\python: The interpreter path C:\Users\mshafer\.pex\venvs\7a1c9fddd44690726e1ace55698aa05bf341787d\1be1ff869bb27ae7c6706e26813ff061a3ee4ea1.5c554b2f67064776bfcb5d4225b30bae\bin\python does not exist.

@benjyw
Copy link
Collaborator

benjyw commented May 31, 2022

BTW are you working off latest main branch? There have been some recent changes to the POSIX locking code in common.py

@benjyw
Copy link
Collaborator

benjyw commented May 31, 2022

Also, not sure if you've already hit this, but this will be necessary:

index e3d15cb..4340001 100644
--- a/pex/dist_metadata.py
+++ b/pex/dist_metadata.py
@@ -151,8 +151,8 @@ def find_dist_info_files(
     # circumstances. This is since we're limiting ourselves to the products of installs by our
     # vendored versions of wheel and pip which turn `-` into `_` as explained in `ProjectName` and
     # `Version` docs.
-    dist_info_metadata_pattern = "^{}$".format(
-        os.path.join(r"(?P<project_name>.+)-(?P<version>.+)\.dist-info", re.escape(filename))
+    dist_info_metadata_pattern = "^(?P<project_name>.+)-(?P<version>.+)\.dist-info{}{}$".format(
+        re.escape(os.path.sep), re.escape(filename)

When embedding an os.path.sep in a regex it has to be escaped properly because backslash has meaning in regexes.

@mshafer-NI
Copy link

BTW are you working off latest main branch?

I merged main into the branch to start (so latest as of 4/15)

Also, not sure if you've already hit this, but this will be necessary:

Good to know. Earliest I'll be able to get back to this will be end of next week.

@g-cassie
Copy link

g-cassie commented Jun 1, 2022

Just got some time to look through my notes. It's been over a year so it's very fuzzy but here's some notes that could be useful.

One problem we encountered when looking at this was that it was not clear you could actually get a .pex file to run on Windows, even if you could build a .pex file on windows. In our case, this was the entire goal. If we still had the requirement to run on Windows we probably would have looked for a way to use a PEX file to bootstrap a virtualenv on a windows box, rather than actually run.

From the PEX docs

PEX files rely on a feature in the Python importer that considers the presence of a __main__.py 
within the module as a signal to treat that module as an executable. For example, python -m 
my_module or python my_module will execute my_module/__main__.py if it exists.

Because of the flexibility of the Python import subsystem, python -m my_module works 
regardless if my_module is on disk or within a zip file. Adding #!/usr/bin/env python to the
 top of a .zip file containing a __main__.py and marking it executable will turn it into an executable
 Python program. pex takes advantage of this feature in order to build executable .pex files. 
This is described more thoroughly in [PEP 441](https://www.python.org/dev/peps/pep-0441/).

I have notes saying you could call pex files like this on Windows python.exe -mpex some.pex, but then in my notes I wrote "how would we handle a test like this" and linked to this test 🤷‍♂️

https://github.com/pantsbuild/pex/blob/3d0e1ca0633edde10a49190cd967e755e88a761c/tests/tools/commands/test_venv.py#L533

In another note, we found based on this SO answer that this works:

assoc .pex=pexfile
ftype pexfile=C:\Users\Administrator\AppData\Local\Programs\Python\Python36\python.exe %1 %*

But i think you have to make that change on the Windows machine where you want to run the pex?

That's all I've got - best of luck!

cognifloyd pushed a commit to cognifloyd/pex that referenced this pull request Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants