Dissable plugins when needed #113
Conversation
…gradle for other things. need to merge in master again
# Conflicts: # pygradle-plugin/src/integTest/groovy/com/linkedin/gradle/python/plugin/PexIntegrationTest.groovy # pygradle-plugin/src/integTest/groovy/com/linkedin/gradle/python/plugin/PythonPluginIntegrationTest.groovy # pygradle-plugin/src/integTest/groovy/com/linkedin/gradle/python/plugin/PythonWebApplicationPluginIntegrationTest.groovy # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/extension/PythonDetails.java # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/extension/VirtualEnvironment.java # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/extension/internal/ExecutablePathUtils.java # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/plugin/PythonPlugin.groovy # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/Flake8Task.groovy # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/PyTestTask.groovy # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/util/FileSystemUtils.java # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/util/OperatingSystem.java # pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/util/internal/pex/PexExecSpecAction.java # pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/PythonExtensionTest.groovy # pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/extension/PythonDetailsUnixTest.groovy # pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/extension/PythonDetailsWindowsTest.groovy # pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/util/internal/ExecutablePathUtilsTest.groovy # version.properties
|
i figured out (i think) why appveyor failed, it seems that each version of python gives a slightly different output text and i was gathering that text to do my checks. But this lead to another problem with the code. For whatever reason, in the base plugin, you version lock pip to 7.X. the problem is i have machines where when it creates the initial vm, python puts in pip 9.x, then pygradle attempts to downgrade it to 7. That downgrade typically fails and the whole integration suite fails. Is there a particular reason not to up the version of pip and setuptools to the latest? what about the others, flake8 is rather old, with many of the other libraries. |
…f pip and setuptools
|
@scphantm There is a reason for pinning the versions of the setup/build/test dependencies. Some packages do not work with higher versions of setuptools or the latest version of flake8 does not support all the versions of Python that we do, ..., etc. We do currently use some of the higher versions internally then what's in the open-source pygradle. |
|
well, ill roll the pip and setuptools back then. and update these in the table. wish i could figure out why uninstalling 9.0.1 causes it to fail. it shouldn't, i don't get it. but i have the test suite running on 3 machines right now, when those cycles finish i will put up another commit. how does the rest of my changes look? |
|
I just don't get it. This suite works find on my mac, but on my windows, i get this. and it only seems to do it with python 3 because python 3 is whats putting in the newer version of pip. do you think the fix is to do a python version sensitive install? meaning if its python 2, install pip 7.12, but if its 3, install 9? |
|
Well, if nothing else, the error is correct, the file its asking for doesn't exist. the only file in that folder is "init.cpython-35.pyc" |
|
@ethankhall my file not found error looks exactly like the one we encountered before with your windows merge. whats really weird is that appveyor worked. When the test creates the initial venv, is it using the exe's from the shim or are there full installations of 3 different versions of python on the appveyor machine. Im really beginning to think its a python 3.5 issue as my windows machines don't have 2.X installed at all, only 3.5 and my macs system default is python 2.7 |
|
@ethankhall , unfortunately, i found it. same file and everything, and sure enough, my path is 263 characters long I've been working on this for 20 hours straight, time for some sleep. I guess we have to change the test framework to use a different temp dir when it runs. shouldn't be horribly difficult, but, Tomorrows problem. |
…lso implemented a way to assign a pip.conf file to a venv so you can redirect pip on a per project, or global project basis to an internal pypi server. Still having windows problems, pip won't pick up the config file for some reason, but i want to see what citravis does with this code, maybe its not me
|
Ok, i had to redo the base project test routines to get them to create a temp folder in a location other than the OS temp folder. Can't really pass parameters to the TemporaryFolder's constructor the way it was setup so i had to abstract it out. but if its windows, it will create the temporary folders in c:\tmp instead of the system temp. This solved the problem of file names exceeding 260 chars (for now). Windows 10 sorta breaks that barrier down but it won't apply to this because the win10 fix only works on managed applications. this isn't managed. I also encountered a big problem with pip. it seems for some reason, occasionally, but not all the time, pip decided it needs to go download setuputils-scm off of pipy.python.org. In my network, that site has a hard block on it. Nothing can get to it, even thru the proxy so the test failed. So I need to set the pip.ini (or pip.conf on unix) to point to our internal server. I put in code and configuration settings that allow you to configure the pip.conf on a per project basis, or if the pip.conf is present, then apparently is spills over to the host OS's pip.conf. I have all of this configured to test and for the life of me I can't get pip to pick up the files. I have read the source of pip and it looks right, my only thought is it may be something to do with such an old version of pip on python 35, which is what i am running. What makes it even weirder is when i go into the pip task and hard code an invalid pypi url with the index-url parameter, it stops attempting to go out of the network all together and works just fine. I'm baffled. Last one is speed. i know windows is slower than linux for files, i get it. but this project now takes 45 minutes to build on my machine. between 6 - 8 minutes for each of the integration tests. Nearly all of that time is spent installing packages into the venv. Does anyone have any ideas how to speed that up? |
| this.details = new PythonDetails(project) | ||
| docsDir = Paths.get(project.projectDir.absolutePath, "docs").toFile().path | ||
| testDir = Paths.get(project.projectDir.absolutePath, "test").toFile().path | ||
| srcDir = Paths.get(project.projectDir.absolutePath, "src").toFile().path | ||
| setupCfg = Paths.get(project.projectDir.absolutePath, "setup.cfg").toFile().path | ||
|
|
||
| // creating a flake8 config file if one doesn't exist, this prevents "file not found" issues. | ||
| def cfgCheck = project.file(setupCfg) |
There was a problem hiding this comment.
Don't do work in configuration time. If you want this, pull it into a task that flake8 depends on.
There was a problem hiding this comment.
done (post just to keep tasks tracked in my head)
| import org.gradle.api.tasks.TaskAction | ||
|
|
||
|
|
||
| class CleanSaveVenvTask extends DefaultTask { |
There was a problem hiding this comment.
We're attempting to move to Java 7 from Groovy (speed and type safety). Can you move this to Java?
There was a problem hiding this comment.
you want it in the src/java or src/groovy folder. seems everyone has a preference now adays.
There was a problem hiding this comment.
I don't really care but looking at the project structure src/main/groovy would probably be most consistent with what we have now.
There was a problem hiding this comment.
done (post just to keep tasks tracked in my head) also added a unit test to run it as well
| import com.linkedin.gradle.python.extension.PythonDetails | ||
| import com.linkedin.gradle.python.tasks.execution.FailureReasonProvider | ||
| import com.linkedin.gradle.python.tasks.execution.TeeOutputContainer | ||
| import groovy.transform.CompileDynamic | ||
| import groovy.transform.CompileStatic | ||
| import org.apache.tools.ant.taskdefs.condition.Os |
There was a problem hiding this comment.
Don't use this. Use our OperatingSystem class.
| fileExtension = "conf" | ||
| } | ||
|
|
||
| File pip = new File(Paths.get(pythonDetails.getVirtualEnv().absolutePath, "pip.${fileExtension}").toString()) |
There was a problem hiding this comment.
Paths.get(pythonDetails.getVirtualEnv(), "pip.${fileExtension}").toFile().getAbsoluteFile()
There was a problem hiding this comment.
done. added unit test to verify creation of the pip file as well
| def pyVersion = pythonDetails.getPythonVersion().pythonMajorMinor | ||
| def extension = ExtensionUtils.getPythonExtension(project) | ||
| def sitePackages = findSitePackages() | ||
|
|
||
| for (File installable : getConfigurationFiles()) { | ||
| if (isReadyForInstall(installable)) { |
There was a problem hiding this comment.
What problem are you trying to solve here?
There was a problem hiding this comment.
we don't use this plugin for python projects (yet, thats phase 2) phase 1 is to enable sphinx documentation for our java, c#, Javascript, and sql projects, several hundred of them. So for a java project that has nothing to do with python, we can't have the plugin fail because it couldn't find a python project. This method is looking to see if pip is getting ready to install the python project itself. If it is, is it properly configured (setup.py exists and such)? If its not properly configured, we throw a warning and move on instead of blow up the build.
There was a problem hiding this comment.
Are you writing your own plugin on top pygradle to do this? If so I would just disable all the tasks that aren't specifically needed for Sphinx.
There was a problem hiding this comment.
no, i have a very thin wrapper that is simply setting some default configuration settings. thats it.
There was a problem hiding this comment.
Ok. Thinking about it, this is probably a pretty useful use case that we solve (docs across languages are hard). I know earlier you proposed making a Sphinx plugin would you be up to doing that? Then you could disable all the things that you don't need to optimize this use case. I would probably still apply the python plugin then configure it to make it fast.
There was a problem hiding this comment.
I can certainly try. are you opposed to me adding https://github.com/dorongold/gradle-task-tree to help in deconstructing the task graph? If you have major concerns with it, i can put it in for development and pull it out before you merge.
There was a problem hiding this comment.
I looked at breaking this up a little closer, that is a big deal, lots of testing, lots of regression testing, lots of ways to screw it up. Lets get this PR kosher and merged, then I will create a second PR to tackle splitting off Sphinx. Trying to do both is a bit much for one PR in my opinion.
| @@ -36,21 +37,33 @@ class PyTestTask extends AbstractPythonTestSourceDefaultTask { | |||
| ignoreExitValue = true | |||
| } | |||
|
|
|||
| def dissableTests() { | |||
There was a problem hiding this comment.
I think we should do this as Task.onlyIf(), so the task doesn't even execute.
| PythonExtension pythonExtension = project.extensions.getByName("python") as PythonExtension | ||
|
|
||
| Map<String, Map<String, String>> pipConfig = pythonExtension.pipConfig | ||
|
|
There was a problem hiding this comment.
Could you refactor this out into a class so a test can be written for it?
There was a problem hiding this comment.
the config or the whole feature? Im still having problems getting pip to read the file for some reason too. I'm beginning to think its the combination of pip 7.X with python 3.5 that im running is whats screwing me up.
There was a problem hiding this comment.
Just the writing out the config file.
There was a problem hiding this comment.
done, pulled it out to a java class, previous test still checks it.
| project.logger.info("Flake8: testDir doesn't exist") | ||
| } | ||
|
|
||
| if (paths.size() > 0) { |
There was a problem hiding this comment.
Task.onlyIf() here too.
There was a problem hiding this comment.
Done, but there was a complication. Narc complained that the PythonPluggins class went over 350 lines (352 to be exact). So to make it smaller, I tried moving the constants to an interface. Been doing that for 15 years now. Apparently, thats not considered a good design pattern anymore cuz Narc complained about that. Turns out, for big lists of constants like that the prefered pattern is an Enum. So i had to convert everything to an enum and update the codebase. When I do the Sphinx conversion, I will split this file up into sub classes. I did it with another plugin I have, its more planning than work. When i do that, i can convert things back to string constants if you want because i will have the room in the file. Otherwise, when doing that conversion I will further refine the Enum. Its a single enum at the moment, really should be 3 or 4. (tasks, groups, directories, etc)
| List<String> sArgs = [pythonDetails.virtualEnvironment.findExecutable("flake8").absolutePath, | ||
| "--config", component.setupCfg] | ||
|
|
||
| if (!project.file(pythonDetails.virtualEnvironment.findExecutable("flake8").absolutePath).exists()) { |
There was a problem hiding this comment.
What's this if-check for? Why would flake8 not be installed?
There was a problem hiding this comment.
It was very strange, i was getting integration tests that were failing with a "file not found". started putting more and more checks into figure out what it coudn't find and magically started working. i can start pulling some of that out and see if it breaks again
| */ | ||
| package com.linkedin.gradle.python.plugin.testutils | ||
|
|
||
| import org.apache.tools.ant.taskdefs.condition.Os |
There was a problem hiding this comment.
Use the OperatingSystem class here too.
| @@ -132,7 +137,7 @@ class PythonExtension { | |||
| forceVersion(split[0], split[1], split[2]) | |||
| } | |||
|
|
|||
| public PythonDetails getDetails() { | |||
| PythonDetails getDetails() { | |||
There was a problem hiding this comment.
Lets leave these methods as public. We use them internally.
| def pyVersion = pythonDetails.getPythonVersion().pythonMajorMinor | ||
| def extension = ExtensionUtils.getPythonExtension(project) | ||
| def sitePackages = findSitePackages() | ||
|
|
||
| for (File installable : getConfigurationFiles()) { | ||
| if (isReadyForInstall(installable)) { |
| @@ -108,6 +108,10 @@ public void args(String... args) { | |||
| arguments.addAll(Arrays.asList(args)); | |||
| } | |||
|
|
|||
| public void args(List<String> args) { | |||
There was a problem hiding this comment.
Do we need this? Shouldn't args(Collection) handle this?
| if (!specificFileGiven) { | ||
| args(component.testDir) | ||
| } | ||
| // if (!project.file(component.testDir).exists()) { |
There was a problem hiding this comment.
Let's remove these commented out lines, and the commented out method too.
…tm/pygradle into DissablePluginsWhenNeeded
We use pygradle primarily as a documentation system for the moment. Because of this, we have projects that won't be configured for full python development. I began going thru the plugin and basically hardening it against things like the setup.py missing doesn't kill the build, just dissables pygradle, same with other aspects. There are tests here to validate