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-194] Use ear archiver instead of jar archiver #9

Closed
wants to merge 8 commits into from

Conversation

santik
Copy link

@santik santik commented Jun 24, 2020

In plugin instead of correct EAR archiver JAR archiver was used.
In this PR application will use correct archiver.

@pzygielo
Copy link
Contributor

On fb2a47c I got

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-ear-plugin:3.1.0-SNAPSHOT:ear (default-ear) on project maven-ear-plugin-test-project-054: Error assembling EAR: Deployment descriptor: .../maven-ear-plugin/target/test-classes/projects/project-054/target/maven-ear-plugin-test-project-054-99.0/META-INF/application.xml does not exist. -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-ear-plugin:3.1.0-SNAPSHOT:ear (default-ear) on project maven-ear-plugin-test-project-054: Error assembling EAR
...
[ERROR] Errors: 
[ERROR]   EarMojoIT.testProject054:652->AbstractEarPluginIT.doTestProject:184->AbstractEarPluginIT.doTestProject:169->AbstractEarPluginIT.doTestProject:144->AbstractEarPluginIT.executeMojo:128->AbstractEarPluginIT.executeMojo:97 » Verification

@santik
Copy link
Author

santik commented Jun 24, 2020

On fb2a47c I got

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-ear-plugin:3.1.0-SNAPSHOT:ear (default-ear) on project maven-ear-plugin-test-project-054: Error assembling EAR: Deployment descriptor: .../maven-ear-plugin/target/test-classes/projects/project-054/target/maven-ear-plugin-test-project-054-99.0/META-INF/application.xml does not exist. -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-ear-plugin:3.1.0-SNAPSHOT:ear (default-ear) on project maven-ear-plugin-test-project-054: Error assembling EAR
...
[ERROR] Errors: 
[ERROR]   EarMojoIT.testProject054:652->AbstractEarPluginIT.doTestProject:184->AbstractEarPluginIT.doTestProject:169->AbstractEarPluginIT.doTestProject:144->AbstractEarPluginIT.executeMojo:128->AbstractEarPluginIT.executeMojo:97 » Verification

You are right. Sometimes we still need to use JAR archiver. Updated the code.

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.

This needs a test

@hboutemy
Copy link
Member

question: what does an ear archiver do differently from a jar archiver that brings concrete benefit, please?

@santik
Copy link
Author

santik commented Sep 20, 2020

@hboutemy it is described in the ticket as showing incorrect message
however I think it is better to use correct implementation instead of generic jar archiver when it is possible

@hboutemy
Copy link
Member

ok, now I get that the message is written by the archiver, not by the plugin
adn from a pure theoretical perspective, a more dedicated archiver can help in some cases but given the plugin worked perfectly with simple jar, I need to clarify if any ear archiver feature won't conflict
but sure, I'll rework the provided patch to fix the current conflict with master, if you don't beat me at it :)

# Conflicts:
#	src/main/java/org/apache/maven/plugins/ear/EarMojo.java
@santik
Copy link
Author

santik commented Sep 21, 2020

@hboutemy sure! I've just resolved conflicts, but if you have anything to add- do it

}
else
{
theArchiver = getJarArchiver();
Copy link
Member

Choose a reason for hiding this comment

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

given there is a test later that checks and fail if the descriptor does not exist, it seems this case is not really useful, isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

file is not required for the jar archiver, but required for ear

Copy link
Member

Choose a reason for hiding this comment

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

I tested and found that starting with JavaEE 5, descriptor is not required: but currently, EarArchiver is implemented in a way that does not support this condition
ok, now I see how we must either improve EarArchiver, either do the workaround you did: I'll merge and document the workaround

Copy link
Author

Choose a reason for hiding this comment

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

ok
I fixed conflicts again

…archiver

# Conflicts:
#	src/main/java/org/apache/maven/plugins/ear/EarMojo.java
@hboutemy
Copy link
Member

merged and updated to document the unexpectedly complex logic for an issue that we supposed was up-for-grabs... :)

thank you everybody for your help and efforts

@hboutemy hboutemy closed this Sep 26, 2020
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.

4 participants