From 5441c1c8e1bca51d76d78283968c181b87de3cab Mon Sep 17 00:00:00 2001 From: Zvezdan Petkovic Date: Sat, 13 Apr 2019 14:53:04 -0700 Subject: [PATCH] Fix the handling of generated code and store overwrite. A generated code, such as rest.li, is also being installed from a directory, not an sdist package. We need to handle the package version number similar to the project directory. Two sub-projects building at the same time can cause the behavior where both of them do not find a wheel in any layer and proceed to build it in their project layers. Then one of them stores it into the host layer before another. The second one should not fail if that wheel is present when it tries to store it in the host layer. Added tests for both cases and a safety belt against null. --- .../python/tasks/action/pip/WheelBuilder.java | 27 +++++++++++++---- .../python/wheel/LayeredWheelCache.java | 29 +++++++++++++++++-- .../tasks/action/pip/WheelBuilderTest.groovy | 25 ++++++++++++++++ .../python/wheel/LayeredWheelCacheTest.groovy | 14 +++++++++ 4 files changed, 86 insertions(+), 9 deletions(-) diff --git a/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilder.java b/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilder.java index e591916a..17f1b193 100644 --- a/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilder.java +++ b/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilder.java @@ -151,18 +151,33 @@ File getPackage(PackageInfo packageInfo, List extraArgs) { String name = packageInfo.getName(); String version = packageInfo.getVersion(); - boolean isProject = isProjectDirectory(packageInfo); + boolean isDirectory = isPackageDirectory(packageInfo); + boolean isProject = isDirectory && project.getProjectDir().equals(packageFile); Optional wheel; /* * Current project is a directory, not a package, so version may be null. - * Compensate for that. + * Set it to project's version. + * The generated code, such as rest.li can also be a path to directory. + * In that case the version will also be null and we need to set it to + * project's version. */ - if (isProject) { - name = project.getName(); + if (isDirectory) { + if (isProject) { + name = project.getName(); + } version = project.getVersion().toString(); } + /* + * Safety belt. + * This should be impossible and prevented by PackageInfo parsing already. + * However, if either of name/version is still null, return the original back. + */ + if (name == null || version == null) { + return packageFile; + } + // set the flag for doPipOperation customBuild = packageSettings.requiresSourceBuild(packageInfo) || packageSettings.isCustomized(packageInfo) @@ -262,10 +277,10 @@ private List cleanupArgs(List args) { return cleanArgs; } - private boolean isProjectDirectory(PackageInfo packageInfo) { + private boolean isPackageDirectory(PackageInfo packageInfo) { File packageDir = packageInfo.getPackageFile(); String version = packageInfo.getVersion(); - return version == null && Files.isDirectory(packageDir.toPath()) && project.getProjectDir().equals(packageDir); + return version == null && Files.isDirectory(packageDir.toPath()); } // Use of pythonEnvironment may hide really customized packages. Catch them! diff --git a/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCache.java b/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCache.java index 16786295..03d441a7 100644 --- a/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCache.java +++ b/pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCache.java @@ -16,18 +16,20 @@ package com.linkedin.gradle.python.wheel; import com.linkedin.gradle.python.extension.PythonDetails; +import org.gradle.api.logging.Logger; +import org.gradle.api.logging.Logging; + import java.io.File; import java.io.IOException; import java.io.Serializable; import java.io.UncheckedIOException; import java.nio.file.Files; +import java.nio.file.StandardCopyOption; import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; -import org.gradle.api.logging.Logger; -import org.gradle.api.logging.Logging; public class LayeredWheelCache implements WheelCache, Serializable { @@ -74,8 +76,29 @@ public void storeWheel(File wheel, WheelCacheLayer wheelCacheLayer) { File cacheDir = layeredCacheMap.get(wheelCacheLayer); if (wheel != null && cacheDir != null) { + /* + * We want the attributes of the file preserved. + * Also, we want to overwrite the existing file when present. + * Although it seems unlikely, because we look for the wheel + * before trying to store it, the following scenario is possible + * and observed in testing. + * + * When two sub-projects do not find the wheel in any layer + * of the cache during build, they both proceed with the build + * into their respective project layers. One of them finishes + * first and also stores the wheel into the host layer, if not + * customized. The second one then must not fail trying to + * overwrite the existing file. + * + * Alternatively, we could check for existence of the target file + * and avoid overwriting. Notice that legacy code in PipWheelAction + * used FileUtils.copyFile from org.apache.commons.io.FileUtils + * which overwrites by default. We chose the same. It's simpler + * and perhaps even safer if this happens under any other scenario. + */ try { - Files.copy(wheel.toPath(), new File(cacheDir, wheel.getName()).toPath()); + Files.copy(wheel.toPath(), new File(cacheDir, wheel.getName()).toPath(), + StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING); } catch (IOException e) { throw new UncheckedIOException(e); } diff --git a/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilderTest.groovy b/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilderTest.groovy index eb25e38b..ef2e318a 100644 --- a/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilderTest.groovy +++ b/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilderTest.groovy @@ -383,6 +383,31 @@ class WheelBuilderTest extends Specification { } } + def 'builds generated code wheel but getting the project version for it'() { + setup: "do not return wheel from cache layers on first attempt" + def execSpec = Mock(ExecSpec) + def fakeWheel = 'fake/project-dir/wheel' + def stubWheelCache = Stub(WheelCache) { + getTargetDirectory() >> Optional.of(new File('fake/project-dir')) + findWheel(!null, !null, !null, WheelCacheLayer.PROJECT_LAYER) >>> [ + Optional.empty(), Optional.of(new File(fakeWheel))] + findWheel(!null, !null, !null, WheelCacheLayer.HOST_LAYER) >> Optional.empty() + } + def wheelBuilder = createWheelBuilder(execSpec, stubWheelCache) + def generatedDir = temporaryFolder.newFolder('build', 'generated', wheelBuilder.project.getName()) + + when: "we request the generated code directory to be built" + def pkg = wheelBuilder.getPackage(PackageInfo.fromPath(generatedDir), []) + + then: "wheel is built and returned for install" + assert pkg.toString() == fakeWheel + 1 * execSpec.commandLine(_) >> { List> args -> + println args + def it = args[0] + assert it[2] == 'wheel' + } + } + def "wheel with matching custom environment is built"() { setup: "return wheel stub from host layer only" def execSpec = Mock(ExecSpec) diff --git a/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCacheTest.groovy b/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCacheTest.groovy index a368ed62..5d17c1d5 100644 --- a/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCacheTest.groovy +++ b/pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCacheTest.groovy @@ -155,4 +155,18 @@ class LayeredWheelCacheTest extends Specification { cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.HOST_LAYER).isPresent() } + def "stores over already present wheel"() { + setup: "put the wheel in both caches" + def wheelFile = new File(otherCache, 'Sphinx-1.6.3-py2.py3-none-any.whl') + wheelFile.createNewFile() + cache.storeWheel(wheelFile) + def wheelInHostLayer = cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.HOST_LAYER).get() + + when: "wheel is stored from host to project layer without raising an exception" + cache.storeWheel(wheelInHostLayer, WheelCacheLayer.PROJECT_LAYER) + + then: "wheel is found in both layers" + cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.PROJECT_LAYER).isPresent() + cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.HOST_LAYER).isPresent() + } }