Add windows support #104
Merged
Add windows support #104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sholsapp
reviewed
Jan 17, 2017
| @@ -0,0 +1,384 @@ | |||
| /* | |||
There was a problem hiding this comment.
I'm surprised this isn't provided by an apache commons library... did you look?
There was a problem hiding this comment.
I didn't. But I haven't seen something like this before either.
There was a problem hiding this comment.
Nooo. Don't do it like that. The gradle developers I have been working with on another project recommend:
+import org.apache.tools.ant.taskdefs.condition.Os
You can look at my PR for examples on use. I reached a stopping point at my pay job today and was going to pick this up again in the morn. I'll take a look at what you did here a bit closer.
This seems really strange to me. I would prefer to build an extremely simple hello world C/C++ application than to depend on Golang just for a test case. Seems based on https://msdn.microsoft.com/en-us/library/ms235639.aspx that it should be pretty easy to do, assuming it is easy to open a developer console. |
|
I thought about using C/C++ but the reason I didn't was because I didn't have a env to build it. I figured that anyone could update dummy exe's in go since the go compiler can cross target to windows with just some ENV args. We don't build the exe's as part of the build, they are (currently) checked in so the only regular use of golang is when we need to update the exes. |
|
+1 on the Go executables seeming strange - they seem to just spit out a string (the version of Python requests). Can you elaborate on their actual purpose? |
|
On *nix boxes we can just echo out "Python x.y" but on windows we need a real exe. Since windows uses the extension to decide how to execute it, an exe must really be an exe not just a CMD script (tried that first). We could use C/C++ to do this, but then when ever we need to update/add to this, we'll have to have very good docs on how to build the environment to do this. Either way we'll need to compile a binary to use for testing. |
|
Can't we do |
|
We do that, the though here was that instead of having to rely on the user's setup, we can just drop some stub exec's that just respond with the version number. |
|
I'm getting test errors i am about to dig into when i do a clean build, but off the hand, you're python.exe doesn't reflect reality very well. You have your mock exe's as i haven't seen a windows machine yet that versions the exe like this. Primarily since these backwards machines don't have symlinks. (well, it does have mklink, but it requires admin access, which means, we don't have symlinks) This is the way its done everywhere i've ever seen And then the version used by default when you type |
|
@scphantm the test copies then out to disk using the pattern /python{version}/Scripts/python{, major, mayor.minor}.exe. we then prefix those search paths. |
|
What test failure are you getting? The integ tests don't work yet |
i tracked down the pex thin, its right, the file really isn't there
theres an init cpython-35.pyc in the folder but not an _implementation.cpython-35.pyc |
|
those are the only six, not bad for a first run. not bad at all. Im digging deeper, but im kinda digging how you did this. |
|
Thanks. I know the integ tests were't working on my VM due to a few things that need to be fixed:
I have't seen your missing file issue before. @sholsapp @sixninetynine @zvezdan have you guys? |
|
I wish i could attach the test result, but with the info thats recorded in it, my auditors would have a stroke. |
deleting the activate file moves me forward again |
Ok, got things compiling sphinx docs on windows (WHOOHOO!!). Now, the next problem. Look at this log. 5:30 to compile documentation is unusable. And this log is the second time i compiled the doc. I can understand doing all the installs into the venv the first time, but is it really necessary subsequent times? What can we do to cut this time down to something reasonable for editing code? Also, i deleted the activate.bat file and ran things again and the vm started, after creating a new activate.bat. It appears that its recording somewhere if it created it or not, and if the two locations don't jive, it errors. but thats a guess. Great progress, thank you. With this i can keep testing and see if i an convince others that this will work. Thanks |
|
@scphantm I've updated the PR to fix your activate problem. Wow those install times are long. PyGradle should cache the installs so the next time it should be really fast. |
|
thanks, ill pull it and give it a shot. i can run it with --debug if you want, i just need a private spot to put the file, i can't post that publicly, shows too much of my machine config. |
|
Debug is a little much, info should be good enough. You can try a private gist? Or I can give you my email and you can email the file. |
|
i emailed them to the address on your profile |
|
Ok, It looked like no caching took place at all. Did you clean up build dir before you ran the second time? Something that's kinda strange is how long it's taking to do the installs. Would you be able to print out the directory tree of the built venv? Maybe the path has changes slightly and we need to fix that. |
|
i did nothing between the command, just one right after the other just the different gradle file is because while this is being worked out, i don't want it effecting the main project. contents of that pygradle are |
|
@scphantm looks like Windows FileSystem is slower than the *nix ones. From a quick google search https://stackoverflow.com/questions/1842798/python-performance-on-windows . I don't know if it's true but it's what I found that could explain it. |
ethankhall
force-pushed
the
add-windows-support
branch
from
703afdb
to
13b9d18
Compare
|
yea, NTFS is many orders of magnitude slower than EXT or HPFS. But thats not the part that bothers me. The part that bothers me is why does it install the dependency libraries into the VENV each time you run it? The whole gradle'esk way of doing things is if it didn't change, skip it. This piece installs these modules when they didn't change. |
|
@scphantm |
|
and thats exactly whats happening. its not skipping for some reason. the logs i sent him show that. I don't mind taking 4 or 5 min for the first run, i can live with that. but not each run, somethings wrong there. |
|
I think I found the issue(s). We are doing Well on Windows the path is |
ethankhall
force-pushed
the
add-windows-support
branch
3 times, most recently
from
a4a8a06
to
373e60f
Compare
|
Ok the tests all work properly now, and we have Windows CI.. If this works the PR should be good to go. |
|
went from 5 minutes to 16 seconds. Looks like the integration tests still fail, at least they do on my machine. The only other optimization point i can see is this Not exactly sure what it does here, but if its possible to do the whole "if nothing changed, skip it" then great. Otherwise, great job!! |
|
i emailed you the report. |
|
Thanks, I have no idea. I'll pass it onto one of the team members that should be able to help better on Monday. |
|
There are actually a few dependencies that are installing each time, this colorama seems to be the worst offender tho. |
zvezdan
requested changes
Jan 25, 2017
| import org.gradle.api.Project; | ||
|
|
||
|
|
||
| public class PexExtension { | ||
|
|
||
| private File pexCache; | ||
| private boolean fatPex = false; | ||
| private boolean fatPex = OperatingSystem.current() == OperatingSystem.WINDOWS; //Defaulting to fat pex's on windows |
There was a problem hiding this comment.
We should use isWindows instead of comparison for equality.
| } | ||
|
|
||
| public static String getPythonApplicationDirectory() { | ||
| return OperatingSystem.current() == OperatingSystem.WINDOWS ? "Scripts" : "bin"; |
There was a problem hiding this comment.
Replace with isWindows again ...
| */ | ||
| project.tasks.create(TASK_SETUP_LINKS) { | ||
| dependsOn project.tasks.getByName(TASK_VENV_CREATE) | ||
| outputs.file(settings.getDetails().activateLink) | ||
|
|
||
| doLast { | ||
| def activateLinkSource = VirtualEnvExecutableHelper.getExecutable(settings, "bin/activate") | ||
| def activateLinkSource = settings.getDetails().virtualEnvironment.getScript( "activate") |
| @@ -40,8 +41,14 @@ public static void makeSymLink(File target, File destination) throws IOException | |||
| * Check if the file exists because the link checking logic in Gradle differs | |||
| * between Linux and OS X machines. | |||
| */ | |||
| if (!Files.exists(destination.toPath())) { | |||
| Files.createSymbolicLink(destination.toPath(), target.toPath()); | |||
| if (OperatingSystem.current() == OperatingSystem.UNIX) { | |||
| String osName = os.toLowerCase(); | ||
| if (osName.contains("windows")) { | ||
| return WINDOWS; | ||
| } else if (osName.contains("mac os x") || osName.contains("darwin") || osName.contains("osx")) { |
There was a problem hiding this comment.
Can you check is macOS Sierra still returns mac os x here or something else.
| @@ -154,4 +154,19 @@ class PexFileUtil { | |||
| return configNames | |||
| } | |||
|
|
|||
| public static String createThinPexFilename(String name) { | |||
| if (OperatingSystem.current() == OperatingSystem.WINDOWS) { | |||
There was a problem hiding this comment.
Replace with isWindows here and below
ethankhall
force-pushed
the
add-windows-support
branch
from
a96d65e
to
f756021
Compare
Here we need to have an exe to run on windows, since golang is easier to build on multiple platforms than rust, I chose it. I'm checking in the exe's since we need them to be able to run without having to install golang everywhere. We could put them up into artifactory so they aren't checked in but it's only 1.6M each. This is something we can discuss before merging. Here I also took the OperatingSystem mostly intact from Gradle. The one change that I did was to remove the extension stipping, since everything shouldn't have one if possible. I split the PythonDetailsTest into PythonDetailsUnixTest and PythonDetailsWindowsTest to make sure that everything can be tested on different platforms.
Things left: - Make the tests actually work. Right now it looks like the exec can run, but windows doesn't honor the shebang line so we have to make the pex's end with .py so they can run :-( - Drinking. This change has a lot in it, mostly because removed the VirtualEnvExecutableHelper file and replaced it with the VirtualEnvironment. The former required a PythonDetails to do anything, so instead of doing that now you just call into details and ask for the venv. All of the methods that were static no loger are, and life is better :-)
If the file exists, we shouldn't copy it again.
Project isn't serializable so we're making sure it doesn't get included in the serialized object.
- Updated the README.md to mention that pex doesn't offically support Windows, Python needs to be in your PATH, and pex's are renamed to .py - Deleted files that are now done by the OperatingSystem class - Added the thin/fat pex renaming into PexFileUtils. - Renamed some tests so they follow the nameing convetnion <CLASS>Test
- Adding appvryor into the CI pipeline, so we can make sure windows continues to work. - Making our path checking to see if we need to install a dep so it handels the windows python dist - Making it where test exec work on windows
ethankhall
force-pushed
the
add-windows-support
branch
from
f756021
to
86b4325
Compare
zvezdan
approved these changes
Jan 26, 2017
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I took a swing at Windows support with some inspiration from the chats in #95.
Here we need to have an exe to run on windows, since golang is easier to
build on multiple platforms than rust, I chose it. I'm checking in the
exe's since we need them to be able to run without having to install
golang everywhere. We could put them up into artifactory so they aren't
checked in but it's only 1.6M each. This is something we can discuss
before merging.
Here I also took the OperatingSystem mostly intact from Gradle. The one
change that I did was to remove the extension stipping, since everything
shouldn't have one if possible.
I split the PythonDetailsTest into PythonDetailsUnixTest and
PythonDetailsWindowsTest to make sure that everything can be tested on
different platforms.
Things left:
Make the tests actually work. Right now it looks like the exec can
run, but windows doesn't honor the shebang line so we have to make the
pex's end with .py so they can run :-(
Drinking.
This change has a lot in it, mostly because removed the
VirtualEnvExecutableHelper file and replaced it with the
VirtualEnvironment. The former required a PythonDetails to do anything,
so instead of doing that now you just call into details and ask for the
venv. All of the methods that were static no longer are, and life is
better :-)
Supersedes #103