Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@shardulm94
shardulm94 committed Apr 8, 2019
1 parent a91bee5 commit 89923c2
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 56 deletions.
16 changes: 4 additions & 12 deletions ...fs-codegen/src/test/java/com/linkedin/transport/codegen/AbstractTestWrapperGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,26 +56,18 @@ void testWrapperGenerator(String udfMetadataFileResource, String expectedSources

getWrapperGenerator().generateWrappers(context);

final Path expectedSourcesOutputPath;
final Path expectedResourcesOutputPath;
try {
expectedSourcesOutputPath = Paths.get(
Path expectedSourcesOutputPath = Paths.get(
Thread.currentThread().getContextClassLoader().getResource(expectedSourcesOutputFolderResource).toURI());
TestUtils.assertDirectoriesAreEqual(_sourcesOutputDir.toPath(), expectedSourcesOutputPath);
if (expectedResourcesOutputFolderResource != null) {
expectedResourcesOutputPath = Paths.get(
Path expectedResourcesOutputPath = Paths.get(
Thread.currentThread().getContextClassLoader().getResource(expectedResourcesOutputFolderResource).toURI());
} else {
// No resources directory specified, so compare to an empty dir
File tmpEmptyResourcesDir = Files.createTempDir();
tmpEmptyResourcesDir.deleteOnExit();
expectedResourcesOutputPath = tmpEmptyResourcesDir.toPath();
TestUtils.assertDirectoriesAreEqual(_resourcesOutputDir.toPath(), expectedResourcesOutputPath);
}
} catch (URISyntaxException e) {
throw new RuntimeException("Error constructing URI", e);
}

TestUtils.assertDirectoriesAreEqual(_sourcesOutputDir.toPath(), expectedSourcesOutputPath);
TestUtils.assertDirectoriesAreEqual(_resourcesOutputDir.toPath(), expectedResourcesOutputPath);
}

abstract WrapperGenerator getWrapperGenerator();
Expand Down
8 changes: 4 additions & 4 deletions transportable-udfs-plugin/src/main/java/com/linkedin/transport/plugin/Defaults.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ private Defaults() {
// The versions of the Transport and supported platforms to apply corresponding versions of the platform dependencies
private static final Properties DEFAULT_VERSIONS = loadDefaultVersions();

private static Properties loadDefaultVersions () {
try(InputStream is =
private static Properties loadDefaultVersions() {
try (InputStream is =
Thread.currentThread().getContextClassLoader().getResourceAsStream("version-info.properties")) {
Properties defaultVersions = new Properties();
defaultVersions.load(is);
Expand Down Expand Up @@ -94,9 +94,9 @@ private static Properties loadDefaultVersions () {
)
);

private static DependencyConfiguration getDependencyConfiguration(DependencyConfigurationType configurationName,
private static DependencyConfiguration getDependencyConfiguration(DependencyConfigurationType configurationType,
String module, String platform) {
return new DependencyConfiguration(configurationName,
return new DependencyConfiguration(configurationType,
module + ":" + DEFAULT_VERSIONS.getProperty(platform + "-version"));
}
}
21 changes: 6 additions & 15 deletions transportable-udfs-plugin/src/main/java/com/linkedin/transport/plugin/SourceSetUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ static SourceDirectorySet getSourceDirectorySet(SourceSet sourceSet, Language la
}
}

private static Configuration getConfigurationForSourceSet(Project project, SourceSet sourceSet,
DependencyConfigurationType configurationName) {
return project.getConfigurations().getByName(getConfigurationNameForSourceSet(sourceSet, configurationName));
static Configuration getConfigurationForSourceSet(Project project, SourceSet sourceSet,
DependencyConfigurationType configurationType) {
return project.getConfigurations().getByName(getConfigurationNameForSourceSet(sourceSet, configurationType));
}

private static String getConfigurationNameForSourceSet(SourceSet sourceSet,
DependencyConfigurationType configurationName) {
DependencyConfigurationType configurationType) {
final String configName;
switch (configurationName) {
switch (configurationType) {
case ANNOTATION_PROCESSOR:
configName = sourceSet.getAnnotationProcessorConfigurationName();
break;
Expand All @@ -61,7 +61,7 @@ private static String getConfigurationNameForSourceSet(SourceSet sourceSet,
configName = sourceSet.getRuntimeOnlyConfigurationName();
break;
default:
throw new UnsupportedOperationException("Configuration " + configurationName + " not supported");
throw new UnsupportedOperationException("Configuration " + configurationType + " not supported");
}
return configName;
}
Expand All @@ -73,15 +73,6 @@ static void addDependencyToConfiguration(Project project, Configuration configur
configuration.withDependencies(dependencySet -> dependencySet.add(project.getDependencies().create(dependency)));
}

/**
* Adds the provided dependencies to the given {@link Configuration}
*/
static void addDependenciesToConfiguration(Project project, Configuration configuration,
Collection<DependencyConfiguration> dependencies) {
dependencies.forEach(
dependency -> addDependencyToConfiguration(project, configuration, dependency.getDependencyString()));
}

/**
* Adds the provided dependency to the appropriate configurations of the given {@link SourceSet}
*/
Expand Down
48 changes: 23 additions & 25 deletions transportable-udfs-plugin/src/main/java/com/linkedin/transport/plugin/TransportPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import org.gradle.api.tasks.testing.Test;
import org.gradle.language.base.plugins.LifecycleBasePlugin;

import static com.linkedin.transport.plugin.DependencyConfigurationType.*;


/**
* A {@link Plugin} to be applied to a Transport UDF module which:
Expand All @@ -45,7 +47,7 @@ public void apply(Project project) {
SourceSet testSourceSet = javaConvention.getSourceSets().getByName("test");

configureBaseSourceSets(project, mainSourceSet, testSourceSet);
Defaults.DEFAULT_PLATFORMS.forEach(config -> configurePlatform(project, config, mainSourceSet, testSourceSet));
Defaults.DEFAULT_PLATFORMS.forEach(platform -> configurePlatform(project, platform, mainSourceSet, testSourceSet));
});
}

Expand All @@ -60,8 +62,7 @@ private void configureBaseSourceSets(Project project, SourceSet mainSourceSet, S
/**
* Configures SourceSets, dependencies and tasks related to each Transport UDF platform
*/
private void configurePlatform(Project project, Platform platform,
SourceSet mainSourceSet, SourceSet testSourceSet) {
private void configurePlatform(Project project, Platform platform, SourceSet mainSourceSet, SourceSet testSourceSet) {
SourceSet sourceSet = configureSourceSet(project, platform, mainSourceSet);
TaskProvider<GenerateWrappersTask> generateWrappersTask =
configureGenerateWrappersTask(project, platform, mainSourceSet, sourceSet);
Expand All @@ -81,17 +82,15 @@ private void configurePlatform(Project project, Platform platform,
* configurations and configures the default dependencies required for compilation and runtime of the wrapper
* SourceSet
*/
private SourceSet configureSourceSet(Project project, Platform platform,
SourceSet mainSourceSet) {
private SourceSet configureSourceSet(Project project, Platform platform, SourceSet mainSourceSet) {
JavaPluginConvention javaConvention = project.getConvention().getPlugin(JavaPluginConvention.class);
Path platformBaseDir =
project.getBuildDir().toPath().resolve(Paths.get("generatedWrappers", platform.getName()));
Path platformBaseDir = Paths.get(project.getBuildDir().toString(), "generatedWrappers", platform.getName());
Path wrapperSourceOutputDir = platformBaseDir.resolve("sources");
Path wrapperResourceOutputDir = platformBaseDir.resolve("resources");

return javaConvention.getSourceSets().create(platform.getName(), sourceSet -> {
/*
Creates a SourceSet and set the source directories. E.g.
Creates a SourceSet and set the source directories for a given platform. E.g. For the Presto platform,
presto {
java.srcDirs = ["${buildDir}/generatedWrappers/sources"]
Expand All @@ -103,20 +102,20 @@ private SourceSet configureSourceSet(Project project, Platform platform,
sourceSet.getResources().setSrcDirs(ImmutableList.of(wrapperResourceOutputDir));

/*
Sets up the configuration for the wrapper SourceSet. E.g.
Sets up the configuration for the platform's wrapper SourceSet. E.g. For the Presto platform,
configurations {
prestoImplementation.extendsFrom mainImplementation
prestoRuntimeOnly.extendsFrom mainRuntimeOnly
}
*/
project.getConfigurations().getByName(sourceSet.getImplementationConfigurationName())
.extendsFrom(project.getConfigurations().getByName(mainSourceSet.getImplementationConfigurationName()));
project.getConfigurations().getByName(sourceSet.getRuntimeOnlyConfigurationName()).extendsFrom(
project.getConfigurations().getByName(mainSourceSet.getRuntimeOnlyConfigurationName()));
SourceSetUtils.getConfigurationForSourceSet(project, sourceSet, IMPLEMENTATION)
.extendsFrom(SourceSetUtils.getConfigurationForSourceSet(project, mainSourceSet, IMPLEMENTATION));
SourceSetUtils.getConfigurationForSourceSet(project, sourceSet, RUNTIME_ONLY)
.extendsFrom(SourceSetUtils.getConfigurationForSourceSet(project, mainSourceSet, RUNTIME_ONLY));

/*
Adds default dependency config for Presto
Adds the default dependencies for the platform. E.g For the Presto platform,
dependencies {
prestoImplementation sourceSets.main.output
Expand All @@ -125,10 +124,8 @@ private SourceSet configureSourceSet(Project project, Platform platform,
}
*/
SourceSetUtils.addDependencyToConfiguration(project,
project.getConfigurations().getByName(sourceSet.getImplementationConfigurationName()),
mainSourceSet.getOutput());
SourceSetUtils.addDependenciesToSourceSet(project, sourceSet,
platform.getDefaultWrapperDependencies());
SourceSetUtils.getConfigurationForSourceSet(project, sourceSet, IMPLEMENTATION), mainSourceSet.getOutput());
SourceSetUtils.addDependenciesToSourceSet(project, sourceSet, platform.getDefaultWrapperDependencies());
});
}

Expand All @@ -139,7 +136,7 @@ private TaskProvider<GenerateWrappersTask> configureGenerateWrappersTask(Project
Platform platform, SourceSet inputSourceSet, SourceSet outputSourceSet) {

/*
Example generateWrapper task for Presto
Creates a generateWrapper task for a given platform. E.g For the Presto platform,
task generatePrestoWrappers {
generatorClass = 'com.linkedin.transport.codegen.PrestoWrapperGenerator'
Expand Down Expand Up @@ -180,7 +177,7 @@ private TaskProvider<Test> configureTestTask(Project project, Platform platform,
SourceSet mainSourceSet, SourceSet testSourceSet) {

/*
Configures the classpath configuration to run platform-specific tests. E.g. for Presto,
Configures the classpath configuration to run platform-specific tests. E.g. For the Presto platform,
configurations {
prestoTestClasspath {
Expand All @@ -195,16 +192,17 @@ private TaskProvider<Test> configureTestTask(Project project, Platform platform,
*/
Configuration testClasspath =
project.getConfigurations().create(platform.getName() + "TestClasspath", config ->
config.extendsFrom(
project.getConfigurations().getByName(testSourceSet.getImplementationConfigurationName()))
config.extendsFrom(SourceSetUtils.getConfigurationForSourceSet(project, testSourceSet, IMPLEMENTATION))
);
SourceSetUtils.addDependencyToConfiguration(project, testClasspath, mainSourceSet.getOutput());
SourceSetUtils.addDependencyToConfiguration(project, testClasspath, testSourceSet.getOutput());
SourceSetUtils.addDependenciesToConfiguration(project, testClasspath,
platform.getDefaultTestDependencies());
platform.getDefaultTestDependencies().forEach(dependencyConfiguration ->
SourceSetUtils.addDependencyToConfiguration(project, testClasspath,
dependencyConfiguration.getDependencyString())
);

/*
Creates the test task for the platform. E.g. For Presto,
Creates the test task for a given platform. E.g. For the Presto platform,
task prestoTest(type: Test, dependsOn: test) {
group 'Verification'
Expand Down

0 comments on commit 89923c2

Please sign in to comment.