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

[MEAR-267] - Fixed detection if JAR module is included into classpath of particular EAR module manifest #19

Merged

Conversation

mabrarov
Copy link
Contributor

@mabrarov mabrarov commented Oct 3, 2020

Fixed detection if JAR module is included into classpath of particular EAR module manifest (like WAR or EJB module manifest). This is required for correct modification of Class-Path entry of manifest of EAR modules when skinnyWars option is turned on.
Note that this PR has no tests (yet) but I tested it manually with feature/MEAR-267_test branch of https://github.com/mabrarov/dockerfile-test:

$ git clone --branch feature/MEAR-267_test https://github.com/mabrarov/dockerfile-test.git && \
mvn -f dockerfile-test/pom.xml clean package -Ddocker.skip -Dmaven-ear-plugin.version=3.1.1-SNAPSHOT

where expected manifest of WAR inside built EAR, i.e. dockerfile-test/ear/target/ear-1.1.7-SNAPSHOT.ear/org.mabrarov.dockerfile-test-war-1.1.7-SNAPSHOT.war/META-INF/MANIFEST.MF looks like:

Manifest-Version: 1.0
Class-Path: lib/org.apache.commons-commons-text-1.7.jar lib/org.apache
 .commons-commons-lang3-3.9.jar
Build-Jdk-Spec: 1.8
Created-By: Maven WAR Plugin 3.3.1
git-commit: e0458e2c48333f2847abfe58b7d1d0b3e23e8941

while original manifest of WAR, i.e. dockerfile-test/war/target/war-1.1.7-SNAPSHOT.war/META-INF/MANIFEST.MF looks like:

Manifest-Version: 1.0
Class-Path: commons-text-1.7.jar commons-lang3-3.9.jar
Build-Jdk-Spec: 1.8
Created-By: Maven WAR Plugin 3.3.1
git-commit: e0458e2c48333f2847abfe58b7d1d0b3e23e8941

@mabrarov mabrarov changed the title [MEAR-267] - Fixed detection if EAR JAR module is included into classpath of particular EAR module manifest [MEAR-267] - Fixed detection if JAR module is included into classpath of particular EAR module manifest Oct 3, 2020
@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch 5 times, most recently from 717110f to bb67e63 Compare October 3, 2020 20:41
@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch from bb67e63 to 968d1f2 Compare October 3, 2020 20:42
@mabrarov
Copy link
Contributor Author

mabrarov commented Oct 3, 2020

Taking into account enhancements in EarMojoIT made in pull request #17 I would create tests for this pull request after that pull request is merged, because it looks like we need those enhancements here too.

@mabrarov
Copy link
Contributor Author

mabrarov commented Oct 3, 2020

Hmm... I made one more test:

$ git clone --branch MEAR-267_Manifest_classpath_modification https://github.com/mabrarov/MEAR-267-issue-demo.git && \
mvn -q -B -f MEAR-267-issue-demo/pom.xml verify -Dmaven-ear-plugin.version=3.1.1-SNAPSHOT 2>&1 | grep WFLYSRV0059

and it demonstrates:

00:35:52,808 WARN  [org.jboss.as.server.deployment] (MSC service thread 1-2) WFLYSRV0059: Class Path entry guava-19.0.jar in /content/MEAR-267.ear/com.leokom-MEAR-267-ejb-1.0-SNAPSHOT.jar  does not point to a valid jar for a Class-Path reference.

that MEAR-267 has multiple root causes and I fixed in this pull request just one of them.

In MEAR-267-issue-demo we have skinnyWars option turned off and it looks like this makes Maven EAR Plugin skipping modification of manifest of EJB JAR.

@mabrarov
Copy link
Contributor Author

mabrarov commented Oct 3, 2020

I fixed 2nd root cause of MEAR-267 in 89816ae:

$ git clone --branch MEAR-267_Manifest_classpath_modification https://github.com/mabrarov/MEAR-267-issue-demo.git && \
mvn -q -B -f MEAR-267-issue-demo/pom.xml verify -Dmaven-ear-plugin.version=3.1.1-SNAPSHOT 2>&1 | grep -c WFLYSRV0059
...
0

@mabrarov
Copy link
Contributor Author

mabrarov commented Oct 4, 2020

I added some integration tests but they do not cover all permutations (i.e. all changed branches) of options like:

  1. skinnyWars
  2. skipClassPathModification
  3. "dirty" build without clean goal execution

@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch 7 times, most recently from 7c253b8 to c926ab0 Compare October 4, 2020 04:15
@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch 2 times, most recently from 446206e to 81d28e2 Compare October 4, 2020 05:14
…ifest entry as used for removal of JAR modules from EAR modules
@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch from 81d28e2 to 3c34038 Compare October 4, 2020 05:16
@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch from 418447d to b5a54dc Compare October 5, 2020 13:34
@mabrarov mabrarov force-pushed the MEAR-267_Manifest_classpath_modification branch from b5a54dc to a8e5826 Compare October 5, 2020 13:37
@mabrarov
Copy link
Contributor Author

What's pending / required for merging this pull request?

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Let me push this through Jenkins

src/main/java/org/apache/maven/plugins/ear/EarMojo.java Outdated Show resolved Hide resolved
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

@mabrarov
Copy link
Contributor Author

jenkins failed: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-ear-plugin/job/mear-267/

Looks like issue of Jenkins with checkout of pull request (https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-ear-plugin/job/mear-267/1/console):

Fetching without tags
Fetching upstream changes from https://gitbox.apache.org/repos/asf/maven-jenkins-lib.git
 > git fetch --no-tags --force --progress -- https://gitbox.apache.org/repos/asf/maven-jenkins-lib.git +refs/heads/*:refs/remotes/origin/* # timeout=10
Checking out Revision c10869c6952fe48d6d66f394802101bdc6d1a537 (master)
 > git config core.sparsecheckout # timeout=10
 > git checkout -f c10869c6952fe48d6d66f394802101bdc6d1a537 # timeout=10
Commit message: "Restored the full maven version matrix default"
First time build. Skipping changelog.
 > git --version # timeout=10
fatal: bad object 4a8c35fa6acb6a409ac027ebf16ae7f3073bbbba

@elharo
Copy link
Contributor

elharo commented Oct 12, 2020

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

@mabrarov
Copy link
Contributor Author

Merge conflicts were fixed

@mabrarov
Copy link
Contributor Author

I just found an error during "dirty" build. Let me try to fix it before proceeding with this PR.

…R modules when doing "dirty" build, i.e. when element with desired path already exists in target manifest Class-Path entry
@mabrarov
Copy link
Contributor Author

I just found an error during "dirty" build. Let me try to fix it before proceeding with this PR.

That was fixed in f070e47. This PR is ready for testing and review now.

@mabrarov
Copy link
Contributor Author

@elharo, could you please trigger one more Jenkins build?

@mabrarov
Copy link
Contributor Author

Test failure when building with JDK 11 - refer to https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-ear-plugin/job/19/2/testReport/ - was fixed in e5ea347

@elharo
Copy link
Contributor

elharo commented Oct 13, 2020

@elharo elharo merged commit 74d28d8 into apache:master Oct 13, 2020
@@ -461,7 +461,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "]" );
FileUtils.copyFile( sourceFile, destinationFile );

if ( skinnyWars && module.changeManifestClasspath() )
if ( module.changeManifestClasspath() && ( skinnyWars || module.getLibDir() == null ) )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks that this change introduces non backward compatible behavior - modification of Class-Path entry of manifest - for SAR and HAR modules. Refer to pull request #24 which is going to fix that (with false default value of skinnyModules option).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants