Skip to content

Commit

Permalink
Fix the handling of generated code and store overwrite.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
zvezdan committed Apr 14, 2019
1 parent 1af9f24 commit 5441c1c
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

0 comments on commit 5441c1c

Please sign in to comment.