Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For Var processing changes in ci.common testing #919

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/gradle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ jobs:
- name: Checkout ci.common
uses: actions/checkout@v3
with:
repository: OpenLiberty/ci.common
repository: arunvenmany-ibm/ci.common
path: ci.common
ref: varProcessing
- name: Checkout ci.ant
uses: actions/checkout@v3
with:
Expand Down Expand Up @@ -124,7 +125,7 @@ jobs:
- name: Clone ci.ant, ci.common, ci.gradle repos to C drive
run: |
cp -r D:/a/ci.gradle/ci.gradle C:/ci.gradle
git clone https://github.com/OpenLiberty/ci.common.git C:/ci.common
git clone https://github.com/arunvenmany-ibm/ci.common.git --branch varProcessing --single-branch C:/ci.common
git clone https://github.com/OpenLiberty/ci.ant.git C:/ci.ant
# Cache mvn/gradle packages
- name: Cache Maven packages
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ compileTestGroovy {
}

def libertyAntVersion = "1.9.16"
def libertyCommonVersion = "1.8.35"
def libertyCommonVersion = "1.8.36-SNAPSHOT"

dependencies {
implementation gradleApi()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,10 +505,9 @@ abstract class AbstractServerTask extends AbstractLibertyTask {
configureMultipleAppsConfigDropins(serverNode)
}

protected ServerConfigDocument getServerConfigDocument(CommonLogger log, File serverXML, File configDir, File bootstrapFile,
Map<String, String> bootstrapProp, File serverEnvFile, boolean giveConfigDirPrecedence, Map<String, File> libertyDirPropertyFiles) throws IOException {
if (scd == null || !scd.getServerXML().getCanonicalPath().equals(serverXML.getCanonicalPath())) {
scd = new ServerConfigDocument(log, serverXML, configDir, bootstrapFile, bootstrapProp, serverEnvFile, giveConfigDirPrecedence, libertyDirPropertyFiles)
protected ServerConfigDocument getServerConfigDocument(CommonLogger log, Map<String, File> libertyDirPropertyFiles) throws IOException {
if (scd == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this seems problematic. Before we were checking to see if the path for the server.xml file changed and if so we recreated the ServerConfigDocument. Now we cannot tell if it changed. Makes me wonder if we should always create a new ServerConfigDocument, but that is a lot of processing that happens.

scd = new ServerConfigDocument(log, libertyDirPropertyFiles)
}

return scd
Expand All @@ -520,8 +519,7 @@ abstract class AbstractServerTask extends AbstractLibertyTask {
if (serverConfigFile != null && serverConfigFile.exists()) {
try {
Map<String,String> props = combinedBootstrapProperties == null ? convertPropertiesToMap(server.bootstrapProperties) : combinedBootstrapProperties;
getServerConfigDocument(new CommonLogger(project), serverConfigFile, server.configDirectory, server.bootstrapPropertiesFile, props, server.serverEnvFile,
false, getLibertyDirectoryPropertyFiles(null));
scd = getServerConfigDocument(new CommonLogger(project), getLibertyDirectoryPropertyFiles(null));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to get props anymore at line 521.

if (scd != null && isLocationFound( scd.getLocations(), fileName)) {
logger.debug("Application configuration is found in server.xml : " + fileName)
configured = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -682,8 +682,7 @@ class DeployTask extends AbstractServerTask {
File serverXML = new File(getServerDir(project).getCanonicalPath(), "server.xml")

try {
scd = getServerConfigDocument(new CommonLogger(project), serverXML, server.configDirectory,
server.bootstrapPropertiesFile, combinedBootstrapProperties, server.serverEnvFile, false, getLibertyDirectoryPropertyFiles(null))
scd = getServerConfigDocument(new CommonLogger(project), getLibertyDirectoryPropertyFiles(null))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to get serverXML anymore at line 682.


//appName will be set to a name derived from appFile if no name can be found.
appName = scd.findNameForLocation(appFile)
Expand Down
78 changes: 38 additions & 40 deletions src/main/groovy/io/openliberty/tools/gradle/tasks/DevTask.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import io.openliberty.tools.common.plugins.util.ServerFeatureUtil
import io.openliberty.tools.common.plugins.util.ServerFeatureUtil.FeaturesPlatforms
import io.openliberty.tools.common.plugins.util.ServerStatusUtil
import io.openliberty.tools.gradle.utils.DevTaskHelper
import io.openliberty.tools.gradle.utils.LooseWarApplication
import org.gradle.api.GradleException
import org.gradle.api.Project
import org.gradle.api.artifacts.Configuration
Expand Down Expand Up @@ -378,14 +377,14 @@ class DevTask extends AbstractFeatureTask {
boolean libertyDebug, boolean pollingTest, boolean container, File containerfile, File containerBuildContext,
String containerRunOpts, int containerBuildTimeout, boolean skipDefaultPorts, boolean keepTempContainerfile,
String mavenCacheLocation, String packagingType, File buildFile, boolean generateFeatures, List<Path> webResourceDirs,
List<ProjectModule> projectModuleList
List<ProjectModule> projectModuleList, Map<String, List<String>> parentBuildGradle
) throws IOException, PluginExecutionException {
super(buildDir, serverDirectory, sourceDirectory, testSourceDirectory, configDirectory, projectDirectory, /* multi module project directory */ projectDirectory,
resourceDirs, changeOnDemandTestsAction, hotTests, skipTests, false /* skipUTs */, false /* skipITs */, skipInstallFeature, artifactId, serverStartTimeout,
verifyAppStartTimeout, appUpdateTimeout, ((long) (compileWait * 1000L)), libertyDebug,
true /* useBuildRecompile */, true /* gradle */, pollingTest, container, containerfile, containerBuildContext, containerRunOpts, containerBuildTimeout, skipDefaultPorts,
null /* compileOptions not needed since useBuildRecompile is true */, keepTempContainerfile, mavenCacheLocation, projectModuleList /* multi module upstream projects */,
false /* recompileDependencies only supported in ci.maven */, packagingType, buildFile, null /* parent build files */, generateFeatures, null /* compileArtifactPaths */, null /* testArtifactPaths */, webResourceDirs /* webResources */
false /* recompileDependencies only supported in ci.maven */, packagingType, buildFile, parentBuildGradle /* parent build files */, generateFeatures, null /* compileArtifactPaths */, null /* testArtifactPaths */, webResourceDirs /* webResources */
);
this.libertyDirPropertyFiles = AbstractServerTask.getLibertyDirectoryPropertyFiles(installDirectory, userDirectory, serverDirectory);
ServerFeatureUtil servUtil = getServerFeatureUtil(true, libertyDirPropertyFiles);
Expand Down Expand Up @@ -500,13 +499,21 @@ class DevTask extends AbstractFeatureTask {
@Override
public boolean updateArtifactPaths(ProjectModule projectModule, boolean redeployCheck, boolean generateFeatures, ThreadPoolExecutor executor)
throws PluginExecutionException {
// not supported for Gradle, only used for multi module Maven projects
// unable to identify the changes made, showing option for user. always return false because action is invoked manually
if (isMultiModuleProject()) {
info("A change was detected in a build file. The libertyDev task could not determine if a server restart is required.");
info("To restart the server, type 'r' and press Enter.");
}
return false;
}

@Override
public boolean updateArtifactPaths(File parentBuildFile) {
// not supported for Gradle, only used for multi module Maven projects
// unable to identify the changes made, showing option for user. always return false because action is invoked manually
if (isMultiModuleProject()) {
info("A change was detected in a build file. The libertyDev task could not determine if a server restart is required.");
info("To restart the server, type 'r' and press Enter.");
}
return false;
}

Expand Down Expand Up @@ -536,6 +543,12 @@ class DevTask extends AbstractFeatureTask {
boolean installFeatures = false;
boolean optimizeGenerateFeatures = false;

if (isMultiModuleProject()) {
info("A change was detected in a build file. The libertyDev task could not determine if a server restart is required.");
info("To restart the server, type 'r' and press Enter.");
return false;
}

ProjectBuilder builder = ProjectBuilder.builder();
Project newProject;
try {
Expand Down Expand Up @@ -1266,14 +1279,19 @@ class DevTask extends AbstractFeatureTask {
// Project modules contain all child modules. This project modules will be present only for multi-module
// used to watch sub project src and test source files
List<ProjectModule> projectModules = getProjectModules()
// get parent build.gradle to register
Map<String, List<String>> parentBuildGradle = new HashMap<String, List<String>>();
if(projectModules.size()>0) {
DevTaskHelper.updateParentBuildFiles(parentBuildGradle, project)
}
try {
this.util = new DevTaskUtil(project.getLayout().getBuildDirectory().getAsFile().get(), serverInstallDir, getUserDir(project, serverInstallDir),
serverDirectory, sourceDirectory, testSourceDirectory, configDirectory, project.getRootDir(),
resourceDirs, changeOnDemandTestsAction.booleanValue(), hotTests.booleanValue(), skipTests.booleanValue(), skipInstallFeature.booleanValue(), artifactId, serverStartTimeout.intValue(),
verifyAppStartTimeout.intValue(), verifyAppStartTimeout.intValue(), compileWait.doubleValue(),
libertyDebug.booleanValue(), pollingTest.booleanValue(), container.booleanValue(), containerfile, containerBuildContext, containerRunOpts,
containerBuildTimeout, skipDefaultPorts.booleanValue(), keepTempContainerfile.booleanValue(), localMavenRepoForFeatureUtility,
DevTaskHelper.getPackagingType(project), buildFile, generateFeatures.booleanValue(), webResourceDirs, projectModules
DevTaskHelper.getPackagingType(project), buildFile, generateFeatures.booleanValue(), webResourceDirs, projectModules, parentBuildGradle
);
} catch (IOException | PluginExecutionException e) {
throw new GradleException("Error initializing dev mode.", e)
Expand Down Expand Up @@ -1416,8 +1434,10 @@ class DevTask extends AbstractFeatureTask {
private List<ProjectModule> getProjectModules() {
List<ProjectModule> upstreamProjects = new ArrayList<ProjectModule>();
for (Project dependencyProject : DevTaskHelper.getAllUpstreamProjects(project)) {
// TODO get compiler options for upstream project
// JavaCompilerOptions upstreamCompilerOptions = getMavenCompilerOptions(p);
// In Maven , there is a step to set compiler options for upstream project
// Gradle does not need to manually inject compiler options because
// we are directly calling compileJava task, which internally takes the compiler options
// from task definition or command line arguments
JavaCompilerOptions upstreamCompilerOptions = new JavaCompilerOptions();
SourceSet mainSourceSet = dependencyProject.sourceSets.main;
SourceSet testSourceSet = dependencyProject.sourceSets.test;
Expand All @@ -1431,40 +1451,18 @@ class DevTask extends AbstractFeatureTask {
File upstreamTestOutputDir = testOutputDirectory.asFile.get();
// resource directories
List<File> upstreamResourceDirs = mainSourceSet.resources.srcDirs.toList();
/* TODO all gradle items
// properties that are set in the pom file
Properties props = dependencyProject.getProperties();

// properties that are set by user via CLI parameters
Properties userProps = session.getUserProperties();

Plugin libertyPlugin = getLibertyPluginForProject(p);
// use "dev" goal, although we don't expect the skip tests flags to be bound to any goal
Xpp3Dom config = ExecuteMojoUtil.getPluginGoalConfig(libertyPlugin, "dev", getLog());

boolean upstreamSkipTests = DevHelper.getBooleanFlag(config, userProps, props, "skipTests");
boolean upstreamSkipITs = DevHelper.getBooleanFlag(config, userProps, props, "skipITs");
boolean upstreamSkipUTs = DevHelper.getBooleanFlag(config, userProps, props, "skipUTs");

// only force skipping unit test for ear modules otherwise honour existing skip
// test params


// build list of dependent modules
List<MavenProject> dependentProjects = graph.getDownstreamProjects(p, true);
List<File> dependentModules = new ArrayList<File>();
for (MavenProject depProj : dependentProjects) {
dependentModules.add(depProj.getFile());
}
*/
boolean upstreamSkipTests = false
boolean upstreamSkipITs = false
boolean upstreamSkipUTs = false
//get gradle project properties. It is observed that project properties contain all gradle properties
// properties are overridden automatically with the highest precedence
// in gradle, we are only using skipTests
boolean upstreamSkipTests = dependencyProject.hasProperty("skipTests") ? DevTaskHelper.parseBooleanIfDefined(dependencyProject.properties.get("skipTests")) : skipTests

if (DevTaskHelper.getPackagingType(dependencyProject).equals("ear")) {
upstreamSkipUTs = true;
}
// build list of dependent modules
// build list of dependent modules -> can be kept as empty list for gradle
// In gradle multi module project, we are calling compileJava for ear
// Then gradle internally identifies other transitive project dependencies and calls compileJava for each of them
// gradle checks whether the task is UP TO DATE, if its already UP TO DATE, it wont be triggered again
List<File> dependentModules = new ArrayList<File>();
ProjectModule upstreamProject = new ProjectModule(dependencyProject.getBuildFile(),
dependencyProject.getName(),
Expand All @@ -1477,8 +1475,8 @@ class DevTask extends AbstractFeatureTask {
upstreamTestOutputDir,
upstreamResourceDirs,
upstreamSkipTests,
upstreamSkipUTs,
upstreamSkipITs,
false,
false,
upstreamCompilerOptions,
dependentModules);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ class StartTask extends AbstractServerTask {
if (serverConfigFile != null && serverConfigFile.exists()) {
try {
Map<String,String> props = combinedBootstrapProperties == null ? convertPropertiesToMap(server.bootstrapProperties) : combinedBootstrapProperties;
getServerConfigDocument(new CommonLogger(project), serverConfigFile, server.configDirectory, server.bootstrapPropertiesFile, props, server.serverEnvFile,
false, getLibertyDirectoryPropertyFiles(null));
scd = getServerConfigDocument(new CommonLogger(project), getLibertyDirectoryPropertyFiles(null));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the line before this needs to be done anymore. The bootstrap.properties file in the server dir will be used and should have all the combined properties already.

if (scd != null) {
appNames = scd.getNames()
appNames += scd.getNamelessLocations().collect { String location ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ class UndeployTask extends AbstractServerTask {
File serverXML = new File(getServerDir(project).getCanonicalPath(), "server.xml")

try {
getServerConfigDocument(new CommonLogger(project), serverXML, server.configDirectory,
server.bootstrapPropertiesFile, combinedBootstrapProperties, server.serverEnvFile, false, getLibertyDirectoryPropertyFiles(null))
scd= getServerConfigDocument(new CommonLogger(project), getLibertyDirectoryPropertyFiles(null))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to get serverXML at line 79 any more.


//appName will be set to a name derived from appFile if no name can be found.
appName = scd.findNameForLocation(file)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,32 @@ public class DevTaskHelper {
retVal.add(war.getWebAppDirectory().get().asFile.toPath().toAbsolutePath())
}
}

/**
* Parses a Boolean from a Object if the Object is not null. Otherwise returns null.
* @param value the Object to parse
* @return a Boolean, or null if value is null
*/
public static Boolean parseBooleanIfDefined(Object value) {
if (value != null) {
return Boolean.parseBoolean(value as String);
}
return null;
}

/**
* Update map with list of parent build files and their subsequent child build files
*
* @param parentBuildFiles Map of parent build files and subsequent child build files
* @param proj GradleProject
*/
public static void updateParentBuildFiles(Map<String, List<String>> parentBuildFiles, Project proj) {
String parentBuildGradle = proj.getRootProject().getBuildFile().getCanonicalPath()
List<String> childBuildFiles = new ArrayList<>();
childBuildFiles.add(proj.getBuildFile().getCanonicalPath())
for (Project dependencyProject : getAllUpstreamProjects(proj)) {
childBuildFiles.add(dependencyProject.getBuildFile().getCanonicalPath())
}
parentBuildFiles.put(parentBuildGradle, childBuildFiles)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class TestMultiModuleLooseEarAppDevMode extends BaseDevTest {
createDir(buildDir);
createTestProject(buildDir, resourceDir, buildFilename);
new File(buildDir, "build").createNewFile();
runDevMode(buildDir)
runDevMode("--skipTests", buildDir)
}

@Test
Expand All @@ -36,7 +36,42 @@ class TestMultiModuleLooseEarAppDevMode extends BaseDevTest {
javaWriter.append(str);
javaWriter.close();

assertTrue(waitForCompilation(targetHelloWorld, lastModified, 6000));
assertTrue(waitForCompilation(targetHelloWorld, lastModified, 123000));
}

@Test
public void manualTestsInvocationTest() throws Exception {
waitLongEnough();
writer.write("\n");
writer.flush();
if (!verifyLogMessage(123000, "Tests will not run on demand for ear because skipTests is set to true")) {
assertTrue(verifyLogMessage(123000, "Tests will not run on demand for ear because skipTests is set to true"));
}
if (!verifyLogMessage(6000, "Tests will not run on demand for jar because skipTests is set to true")) {
assertTrue(verifyLogMessage(6000, "Tests will not run on demand for jar because skipTests is set to true"));
}
if (!verifyLogMessage(6000, "Tests will not run on demand for war because skipTests is set to true")) {
assertTrue(verifyLogMessage(6000, "Tests will not run on demand for war because skipTests is set to true"));
}

}

@Test
public void modifyUpdateGradleTest() throws Exception {
waitLongEnough();
// modify a java file
File srcHelloWorld = new File(buildDir, "build.gradle");
assertTrue(srcHelloWorld.exists());

String str = "// testing";
BufferedWriter javaWriter = new BufferedWriter(new FileWriter(srcHelloWorld, true));
javaWriter.append(' ');
javaWriter.append(str);
javaWriter.close();

if (!verifyLogMessage(123000, "A change was detected in a build file. The libertyDev task could not determine if a server restart is required.")) {
assertTrue(verifyLogMessage(123000, "A change was detected in a build file. The libertyDev task could not determine if a server restart is required."));
}
}

@AfterClass
Expand All @@ -47,4 +82,5 @@ class TestMultiModuleLooseEarAppDevMode extends BaseDevTest {
System.out.println(stderr);
cleanUpAfterClass(true);
}

}
Loading