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

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
92ca1b8
changes for picking skipTests gradle.property argument passed for su…
arunvenmany-ibm Nov 4, 2024
8a321b4
changes for picking parent build.gradle
arunvenmany-ibm Nov 4, 2024
30a0cc4
changes for showing option to restart or recompile when a change is …
arunvenmany-ibm Nov 4, 2024
9e6cada
Changing absolutePath to cannoicalPath to see whether java 21 windows…
arunvenmany-ibm Nov 5, 2024
319ef84
Changing timeouts
arunvenmany-ibm Nov 5, 2024
51dedff
Changing timeouts
arunvenmany-ibm Nov 5, 2024
19c0f0c
Changing timeouts
arunvenmany-ibm Nov 5, 2024
5c6d2b9
Changing workflow yaml to test only failed class
arunvenmany-ibm Nov 5, 2024
8a5d551
rolling back workflow yaml since all new tests are success
arunvenmany-ibm Nov 5, 2024
1a7a7fc
incorparating review comments
arunvenmany-ibm Nov 7, 2024
36ba635
changes in workflow to build custom ci common branch to run tests
arunvenmany-ibm Nov 7, 2024
85b8432
workflow yaml changes to test ci comon varProcessing branch with ci.…
arunvenmany-ibm Nov 8, 2024
d9a2f34
changing reompileDependencies to true and update artifcats to true fo…
arunvenmany-ibm Nov 11, 2024
ae23c0c
varprocessing incorparate review comments
arunvenmany-ibm Nov 11, 2024
2f793d9
changing message incorparate review comments
arunvenmany-ibm Nov 11, 2024
62fd7e2
Merge pull request #918 from arunvenmany-ibm/gradle_multi_module_2.0
arunvenmany-ibm Nov 12, 2024
5d61caf
changes in workflow to build custom ci common branch to run tests
arunvenmany-ibm Nov 7, 2024
32b8084
workflow yaml changes to test ci comon varProcessing branch with ci.…
arunvenmany-ibm Nov 8, 2024
0979f4f
varprocessing incorparate review comments
arunvenmany-ibm Nov 11, 2024
da619ef
Merge remote-tracking branch 'origin/varProcessing' into varProcessing
arunvenmany-ibm Nov 12, 2024
5cb19b0
testing after removing deprecated constructor
arunvenmany-ibm Nov 12, 2024
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this logic back. I think we should keep the logic

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to keep server.xml comparison logic if condition

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back logic in getServerDocument() to compare server.xml


//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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back logic in getServerDocument() to compare server.xml


//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