Merge pull request #295 from zvezdan/fix-generated-and-stored

Fix the handling of generated code and store overwrite.

pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilder.java

@@ -151,18 +151,33 @@ File getPackage(PackageInfo packageInfo, List<String> 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<File> 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<String> cleanupArgs(List<String> 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!

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);
             }

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<List<String>> 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)

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()
+    }
 }