Merge pull request #296 from zvezdan/fix-store-wheel

Fix store of the present wheel.

pygradle-plugin/src/main/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCache.java

@@ -23,14 +23,16 @@
 import java.io.IOException;
 import java.io.Serializable;
 import java.io.UncheckedIOException;
+import java.nio.file.FileAlreadyExistsException;
 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 static java.nio.file.StandardCopyOption.COPY_ATTRIBUTES;
+
 
 public class LayeredWheelCache implements WheelCache, Serializable {
 
@@ -77,28 +79,28 @@ public void storeWheel(File wheel, WheelCacheLayer 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
+             * Although it seemed 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.
+             * When two (or more) sub-projects do not find the wheel in any
+             * layer of the cache during build, they all proceed with the
+             * build into their respective project layers. One of them
+             * finishes first and stores the wheel into the host layer,
+             * if wheel build is not customized. The others then do not
+             * need to store the same wheel.
+             *
+             * We do not try to overwrite the existing file, but instead
+             * catch the exception and log it. The try-catch avoids race
+             * conditions here much better than conditional expressions.
+             * We re-throw other exceptions.
              *
-             * 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.
+             * The file attributes are preserved after copy.
              */
             try {
-                Files.copy(wheel.toPath(), new File(cacheDir, wheel.getName()).toPath(),
-                    StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING);
+                Files.copy(wheel.toPath(), new File(cacheDir, wheel.getName()).toPath(), COPY_ATTRIBUTES);
+            } catch (FileAlreadyExistsException e) {
+                logger.info("Wheel {} already stored in {}", wheel.getName(), cacheDir.toString());
             } catch (IOException e) {
                 throw new UncheckedIOException(e);
             }

pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/tasks/action/pip/WheelBuilderTest.groovy

@@ -59,7 +59,7 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("foo-1.0.0.tar.gz"), [])
 
         then: "we get the wheel from project layer without any rebuilding"
-        assert pkg.toString() == expected
+        pkg.toString() == expected
         0 * execSpec._
     }
 
@@ -81,8 +81,8 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("foo-1.0.0.tar.gz"), [])
 
         then: "we get the wheel from host layer without any rebuilding and store it into project layer too"
-        assert pkg.toString() == expected
-        assert storeCounter == 1
+        pkg.toString() == expected
+        storeCounter == 1
         0 * execSpec._
     }
 
@@ -99,7 +99,7 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("foo-1.0.0.tar.gz"), [])
 
         then: "wheel is built but if it's not present after that, the source package is returned as fallback"
-        assert pkg == packageInGradleCache("foo-1.0.0.tar.gz").getPackageFile()
+        pkg == packageInGradleCache("foo-1.0.0.tar.gz").getPackageFile()
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
             def it = args[0]
@@ -126,8 +126,8 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("foo-1.0.0.tar.gz"), [])
 
         then: "wheel is built excluding PythonEnvironment, then stored into host layer and returned"
-        assert pkg.toString() == expected
-        assert storeCounter == 1
+        pkg.toString() == expected
+        storeCounter == 1
         1 * execSpec.environment([:])
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
@@ -152,7 +152,7 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("foo-1.0.0.tar.gz"), ["--upgrade", "--ignore-installed"])
 
         then: "wheel is built and there are no incompatible options"
-        assert pkg.toString() == expected
+        pkg.toString() == expected
         1 * execSpec.environment([:])
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
@@ -299,7 +299,7 @@ class WheelBuilderTest extends Specification {
             assert it[2] == 'wheel'
             assert !it.any { entry -> entry == '--ignore-installed' }
         }
-        assert storeCounter == 0
+        storeCounter == 0
     }
 
     def 'does not rebuild if customized but present'() {
@@ -320,8 +320,8 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("pyflakes-1.6.0.tar.gz"), [])
 
         then: "we do not rebuild it or store it into host layer"
-        assert pkg.toString() == expected
-        assert storeCounter == 0
+        pkg.toString() == expected
+        storeCounter == 0
         0 * execSpec._
     }
 
@@ -343,8 +343,8 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("pyflakes-1.6.0.tar.gz"), [])
 
         then: "we build it and get the wheel, but do not store it into host layer"
-        assert pkg.toString() == expected
-        assert storeCounter == 0
+        pkg.toString() == expected
+        storeCounter == 0
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
             def it = args[0]
@@ -374,8 +374,8 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(PackageInfo.fromPath(wheelBuilder.project.getProjectDir()), [])
 
         then: "wheel is built but not stored to host layer and project directory returned for editable install"
-        assert pkg.toString() == wheelBuilder.project.getProjectDir().toString()
-        assert storeCounter == 0
+        pkg.toString() == wheelBuilder.project.getProjectDir().toString()
+        storeCounter == 0
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
             def it = args[0]
@@ -400,7 +400,7 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(PackageInfo.fromPath(generatedDir), [])
 
         then: "wheel is built and returned for install"
-        assert pkg.toString() == fakeWheel
+        pkg.toString() == fakeWheel
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
             def it = args[0]
@@ -428,8 +428,8 @@ class WheelBuilderTest extends Specification {
         def pkg = wheelBuilder.getPackage(packageInGradleCache("numpy-12.0.0.tar.gz"), [])
 
         then: "we do not get it from the host layer but build the custom wheel instead and get it from project layer"
-        assert pkg.toString() == fakeWheel
-        assert storeCounter == 0
+        pkg.toString() == fakeWheel
+        storeCounter == 0
         1 * execSpec.commandLine(_) >> { List<List<String>> args ->
             println args
             def it = args[0]

pygradle-plugin/src/test/groovy/com/linkedin/gradle/python/wheel/LayeredWheelCacheTest.groovy

@@ -155,15 +155,15 @@ class LayeredWheelCacheTest extends Specification {
         cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.HOST_LAYER).isPresent()
     }
 
-    def "stores over already present wheel"() {
+    def "store over already present wheel does not fail"() {
         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()
+        def wheelCopy = cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.PROJECT_LAYER).get()
 
         when: "wheel is stored from host to project layer without raising an exception"
-        cache.storeWheel(wheelInHostLayer, WheelCacheLayer.PROJECT_LAYER)
+        cache.storeWheel(wheelCopy, WheelCacheLayer.HOST_LAYER)
 
         then: "wheel is found in both layers"
         cache.findWheel('Sphinx', '1.6.3', pythonDetails, WheelCacheLayer.PROJECT_LAYER).isPresent()