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-153] skinnyModules option #24

Closed
wants to merge 2 commits into from
Closed

Conversation

mabrarov
Copy link
Contributor

@mabrarov mabrarov commented Oct 14, 2020

Changes:

  • [MEAR-153] - Support of skinnyModules option for WAR, SAR, HAR, RAR and WSR EAR modules.
  • [MEAR-153] - Configurable library directory for each EAR module overriding default value which depends on EAR module type.
  • [MEAR-153] - Documentation for skinny modules and libDirectory property of EAR modules.

There are EAR module types (like AOP and ESB) which:

I don't think that MEAR-153 covers these EAR module types, so I have no plans to implement support of skinnyModules options for these types of EAR modules.

Support of WSR module type is implemented as part of RAR support (WsrModule extends RarModule). I cannot test it yet, because I have no information about WSR packaging / EAR module type.

@mabrarov mabrarov changed the title MEAR-153 skinnyWars or SAR and HAR modules [MEAR-153] skinnyWars or SAR and HAR modules Oct 14, 2020
@mabrarov mabrarov marked this pull request as draft October 15, 2020 00:49
@mabrarov mabrarov changed the title [MEAR-153] skinnyWars or SAR and HAR modules [MEAR-153] skinnyModules option Oct 15, 2020
@mabrarov mabrarov force-pushed the MEAR-153 branch 7 times, most recently from 06a5906 to ed2e25b Compare October 15, 2020 05:12
@mabrarov mabrarov marked this pull request as ready for review October 15, 2020 10:32
@mabrarov
Copy link
Contributor Author

mabrarov commented Oct 15, 2020

We need smbd to test skinny RAR, HAR and SAR - I have no code with this kind of EAR modules to ensure that skinny RAR, HAR and SAR works correctly, i.e. are deployed with the same success level as non-skinny.

@mabrarov
Copy link
Contributor Author

@elharo, I do not plan any further work on or testing of these changes. Could you please trigger Jenkins build and review this pull request? Please note that these changes also fix backward incompatibility introduced by MEAR-267 (by pull request #19 )

@mabrarov mabrarov force-pushed the MEAR-153 branch 3 times, most recently from 2c6c1c5 to eff390b Compare October 15, 2020 17:28
@mabrarov mabrarov force-pushed the MEAR-153 branch 9 times, most recently from 3f80ca3 to 741c9a3 Compare October 16, 2020 17:18
@mabrarov
Copy link
Contributor Author

@elharo, could you please trigger Jenkins build for this pull request?

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 3, 2020

What needs to be done for merging this pull request or what is missing / pending in this pull request? Is there something I have to add to pass review and merge this pull request?

@elharo
Copy link
Contributor

elharo commented Nov 6, 2020

At this point, you need to locate a Java EE savvy committer who understands the details of the EAR format and what this plugin is used for to review and approve. I don't use this plugin or Java EE so I can't help with that part.

@hboutemy
Copy link
Member

hboutemy commented Nov 11, 2020

sorry, I have been away these times...
FYI, I'm not a EAR user either, then I don't have strong opinion on use cases, sorry. But I want to help people doing hard and good work like this one (unit tests, integration tests, documentation).
I have 2 questions to evaluate before merging:

  1. IIUC, the new feature is triggered by a specific parameter: then can't break existing users configuration?
  2. shouldn't we update the version to 3.2.0?

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 11, 2020

Hi @hboutemy,

the new feature is triggered by a specific parameter: then can't break existing users configuration?

Yes. That's one of my intentions - changes should not impact existing projects and the new behavior should work only for users who want / need and explicitly requested it through configuration.

shouldn't we update the version to 3.2.0?

I believe version needs to be changed (3.2.0 looks like a good candidate), but I don't know how it is performed in this project. I thought version change is performed by maintainers when decision on new release is made.

I'm not a EAR user either

I am still an EAR user due to my company legacy tasks which will keep using EARs for at least next 10 years :( Just curious who else can help. dev@maven.apache.org keeps silent. Maybe I need to try users@maven.apache.org ?

Waiting for the results of Roland Asmann's testing, but I doubt we will get them soon.

@hboutemy
Copy link
Member

great: yes, questioning on users@maven.apache.org seems a good option

defining the next version happens when working on an issue where it becomes clear that we're beyond bugfix: yours is like this
Please update your PR with that 3.2.0-SNAPSHOT, I'll do the Jira part

I'm waiting for your feedback to merge this PR, then also to start a release

Thanks a lot, it's great to get such contribution

@mabrarov
Copy link
Contributor Author

@hboutemy

then also to start a release

Please take a look at pull request #22 before that.

@mabrarov
Copy link
Contributor Author

@hboutemy,

Please update your PR with that 3.2.0-SNAPSHOT, I'll do the Jira part

PR updated as requested.

Copy link
Contributor

@rfscholte rfscholte left a comment

Choose a reason for hiding this comment

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

Overall a very nice improvement and it matched my idea that a skinnyWar (as defined in the maven-war-plugin) is not the right apporach when working with ears, it is the maven-ear-plugin its responsibiltiy to extract the libs.
One thing we need to keep in mind is that there will be a request to select per module which artifacts should be moved to the (shared) lib directory. Would such a change fit in this PR without adjusting the new parameters? For example, the suggested skinnyModules is a boolean (all or nothing), which makes it too hard to finetune such requirements, hence I don't think a global skinnyModules is the right approach here.

{{{./ear-mojo.html#skipClassPathModification}skipClassPathModification}} parameter
is false.

If <<<libDirectory>>> is not <<<null>>> and if
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes we wonder if the explicit skinnyModules parameter is required. Setting the libDirectory seems enough to understand that artifacts from the modules are moved here.

Copy link
Contributor Author

@mabrarov mabrarov Nov 15, 2020

Choose a reason for hiding this comment

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

Sorry, but I didn't get your note.

libDirectory here is a property of EAR module. It's not the target location where the shared libraries of skinny EAR module are moved (refer to defaultLibBundleDir option of plugin configuration). It's the source location inside EAR module from which shared libraries (libraries of EAR module which are among EAR dependencies) are moved.

libDirectory has default value depending on type of EAR module, so it's not always null. The need of libDirectory property comes from modules with non-standard layout. For example, JBoss Packaging Maven Plugin can package SAR putting libraries into a user defined directory of SAR (i.e. not into the default root of SAR).

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 15, 2020

@rfscholte,

Regarding your question.

there will be a request to select per module which artifacts should be moved to the (shared) lib directory.

Do you mean "select skinny mode (move all or nothing) per module and not for all modules"?

If yes, then here are my thoughts:

I thought about the same but then decided that skinnyWars option doesn't provide such ability so that skinnyModules option can follow the same approach.

Would such a change fit in this PR without adjusting the new parameters?

We can add both:

  1. skinnyModules option of plugin configuration (implemented)
  2. skinny property of EAR module which default value is equal to skinnyModules option of plugin configuration (skinnyModules || skinnyWars for web module). skinny == false will work the same way (will lead to the same) as when libDirectory == null (it's not possible to specify null value for libDirectory property of EAR module at the moment).

That way we can use global configuration (when we need just all modules) and choose what modules to make "skinny / non-skinny" when required:

  • skinnyModules == true in configuration and skinny == false for the modules which need to be non-skinny,
  • or default / omitted skinnyModules and skinny == true for the modules which need to be skinny.

If you mean choosing subset of EAR module libraries to move into EAR shared lib directory, then we can add this feature too by implementing some artifact filter (skinnyLibFilter) as a new property of EAR module (skinnyLibFilter property could even eliminate need of skinny property), but this looks more complicated (to understand and to implement).

My main concern against making all these changes (which complicate implementation and understanding) is that I'm not sure if this feature is really wanted by the users of Maven EAR Plugin - I never had a need of anything like that:

  1. to move into shared EAR lib directory just a part of EAR module libraries (refer to the note at the end of this comment)
  2. to make some modules of EAR skinny and to keep other modules non-skinny.

Please, share more details about your thoughts and use cases. Maybe we can implement them as a new feature request in a separate pull request based on this pull request. Maybe - it depends on how often these use cases are needed - we can implement them right here, in this pull request. All known to me projects building EARs don't need some EAR modules to be non-skinny or "partially skinny" - all of them need all modules to be just skinny.

Note that if I need to keep just a specific library (non-skinny-lib.jar) in my skinny EAR modules (rare case, but I know at least one of them - we workaround that case by eliminating the need of non-skinny-lib.jar) then I just don't add the non-skinny-lib.jar into the EAR dependencies:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>ear</groupId>
        <artifactId>maven-ear-plugin-test-project-092-parent</artifactId>
        <version>99.0</version>
    </parent>
    <artifactId>maven-ear-plugin-test-project-092</artifactId>
    <packaging>ear</packaging>
    <dependencies>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>war-sample-three</artifactId>
            <type>war</type>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>war-sample-three</artifactId>
            <type>pom</type>
            <exclusions>
                <exclusion>
                    <groupId>non-skinny-lib</groupId>
                    <artifactId>non-skinny-lib</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>sar-sample-two</artifactId>
            <type>sar</type>
            <exclusions>
                <exclusion>
                    <groupId>non-skinny-lib</groupId>
                    <artifactId>non-skinny-lib</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>rar-sample-one</artifactId>
            <type>rar</type>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>rar-sample-one</artifactId>
            <type>pom</type>
            <exclusions>
                <exclusion>
                    <groupId>non-skinny-lib</groupId>
                    <artifactId>non-skinny-lib</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
    </dependencies>
    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-ear-plugin</artifactId>
                <configuration>
                    <version>6</version>
                    <defaultLibBundleDir>lib</defaultLibBundleDir>
                    <skinnyModules>true</skinnyModules>
                </configuration>
            </plugin>
        </plugins>
    </build>
</project>

@rfscholte
Copy link
Contributor

Just as an example: suppose you have several wars in an EAR and you know that they all use the same logging framework, so these artifacts are the once you want to extract from the WAR and put it in the lib of the EAR. it might also another set of artifacts. The choice of doing a subset and not all might have to do with the classloading strategies.

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 15, 2020

suppose you have several wars in an EAR and you know that they all use the same logging framework, so these artifacts are the once you want to extract from the WAR and put it in the lib of the EAR. it might also another set of artifacts.

Just don't add non-shared libraries into EAR dependencies - refer to the note at the end of my previous comment. I understand that it looks like a workaround, so if it's a real case with existing workaround, then I'd prefer to work on it in a dedicated feature request / pull request. It seems to be complicated for me to implement filtering of dependencies of EAR modules (if specific dependency should be moved out of the EAR module or not in case of skinnyModules).

@rfscholte
Copy link
Contributor

Just don't add non-shared libraries into EAR dependencies
The world isn't that perfect, you can't always control that, especially when working with third party artifacts. There is a reason why this part of the spec isn't implemented for the plugin, it is very difficult to do it right. And Maven is missing some optimizations. Most ear dependencies shouldn't download transitive dependencies. The work around is adding exclusions with wildcards. But that's only an instruction for Maven to download, it is not an instruction for the maven-ear-plugin what not to include.
I'll need some time to see if there is a better way to solve this.

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 15, 2020

But that's only an instruction for Maven to download, it is not an instruction for the maven-ear-plugin what not to include.

I agree, but it's the way Maven EAR Plugin is implemented at the moment. Library is not considered "shared" until it is found among dependencies of EAR. It looks like implementer of skinnyWars option cut corners.

Note that we have MEAR-166 which implementation can be connected with (impacted by) your suggestion.

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 15, 2020

Proposal for the further work on design of shared libraries inclusion / exclusion (i.e. this comment is not directly related to this pull request). Backward compatible with this pull request and skinnyWars option.

eartest-war1-xxx.war
└───WEB-INF/lib/
    ├─── group1-lib1-xxx.jar
    ├─── group1-lib2-xxx.jar
    ├─── group2-lib1-xxx.jar
    ├─── group2-lib2-xxx.jar
    └─── group2-lib3-xxx.jar

eartest-war2-xxx.war
└───WEB-INF/lib/
    ├─── group1-lib1-xxx.jar
    ├─── group1-lib2-xxx.jar
    ├─── group2-lib1-xxx.jar
    ├─── group2-lib2-xxx.jar
    └─── group2-lib3-xxx.jar

eartest-rar1-xxx.rar
├─── group1-lib1-xxx.jar
├─── group1-lib2-xxx.jar
├─── group2-lib1-xxx.jar
├─── group2-lib2-xxx.jar
└─── group2-lib3-xxx.jar

eartest-rar2-xxx.rar
├─── group1-lib1-xxx.jar
└─── group1-lib2-xxx.jar
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <groupId>eartest</groupId>
    <artifactId>ear</artifactId>
    <version>xxx</version>
    <packaging>ear</packaging>
    <dependencies>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>war1</artifactId>
            <version>xxx</version>
            <type>war</type>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>war1</artifactId>
            <version>xxx</version>
            <type>pom</type>
            <exclusions>
                <!-- Global exclusion for skinnyWars and skinnyModules -->
                <exclusion>
                    <groupId>group2</groupId>
                    <artifactId>lib3</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>war2</artifactId>
            <version>xxx</version>
            <type>war</type>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>rar1</artifactId>
            <version>xxx</version>
            <type>rar</type>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>rar1</artifactId>
            <version>xxx</version>
            <type>pom</type>
            <exclusions>
                <!-- Global exclusion for skinnyWars and skinnyModules -->
                <exclusion>
                    <groupId>group2</groupId>
                    <artifactId>lib3</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>rar2</artifactId>
            <version>xxx</version>
            <type>rar</type>
        </dependency>
        <dependency>
            <groupId>eartest</groupId>
            <artifactId>rar2</artifactId>
            <version>xxx</version>
            <type>pom</type>
            <exclusions>
                <!-- Global exclusion for skinnyWars and skinnyModules -->
                <exclusion>
                    <groupId>group2</groupId>
                    <artifactId>lib3</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
    </dependencies>
    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-ear-plugin</artifactId>
                <configuration>
                    <skinnyModules>true</skinnyModules>
                    <modules>
                        <webModule>
                            <groupId>eartest</groupId>
                            <artifactId>war1</artifactId>
                            <sharedLibs>
                                <!-- Used only if skinnyWars or skinnyModules is turned on -->
                                <inlcusions>
                                    <inclusion>
                                        <groupId>group1</groupId>
                                        <artifactId>lib1</artifactId>
                                    </inclusion>
                                </inlcusions>
                            </sharedLibs>
                        </webModule>
                        <webModule>
                            <groupId>eartest</groupId>
                            <artifactId>war2</artifactId>
                            <sharedLibs>
                                <!-- Should by non-skinny -->
                                <exclusions>
                                    <exclusion>
                                        <groupId>*</groupId>
                                        <artifactId>*</artifactId>
                                    </exclusion>
                                </exclusions>
                            </sharedLibs>
                        </webModule>
                        <rarModule>
                            <groupId>eartest</groupId>
                            <artifactId>rar1</artifactId>
                            <sharedLibs>
                                <!-- Used only if skinnyModules is turned on -->
                                <exclusions>
                                    <exclusion>
                                        <groupId>group2</groupId>
                                        <artifactId>*</artifactId>
                                    </exclusion>
                               </exclusions>
                            </sharedLibs>
                        </rarModule>
                    </modules>
                </configuration>
            </plugin>
        </plugins>
    </build>
</project>
eartest-ear-xxx.ear
├─── lib
│    ├─── group1-lib1-xxx.jar
│    ├─── group1-lib2-xxx.jar
│    ├─── group2-lib1-xxx.jar (Why? Because it's among dependencies of eartest:ear)
│    └─── group2-lib2-xxx.jar (Why? Because it's among dependencies of eartest:ear)
│
├─── eartest-war1-xxx.war
│    └─── WEB-INF/lib/
│         ├─── group1-lib2-xxx.jar
│         ├─── group2-lib1-xxx.jar
│         ├─── group2-lib2-xxx.jar
│         └─── group2-lib3-xxx.jar
│
├─── eartest-war2-xxx.war
│    └─── WEB-INF/lib/
│         ├─── group1-lib1-xxx.jar
│         ├─── group1-lib2-xxx.jar
│         ├─── group2-lib1-xxx.jar
│         ├─── group2-lib2-xxx.jar
│         └─── group2-lib3-xxx.jar
│
├─── eartest-rar1-xxx.rar
│    ├─── group2-lib1-xxx.jar
│    ├─── group2-lib2-xxx.jar
│    └─── group2-lib3-xxx.jar
│
└─── eartest-rar2-xxx.rar (all libraries matching EAR dependencies are consider shared by default if sharedLibs property is not specified)

@rfscholte
Copy link
Contributor

I had a look at https://maven.apache.org/plugins/maven-ear-plugin/examples/skinny-wars.html to understand the current implementation. It is a little bit different to what I was expecting, but it makes sense.
Your last proposal with the sharedLibs per modules combined with includes/excludes is what I was expecting and give maximum control.

@mabrarov
Copy link
Contributor Author

mabrarov commented Nov 19, 2020

@hboutemy,

The "Maven EAR Plugin 3.2.0 with skinnyModules option" topic I started in the users@maven.apache.org mailing list has no answers for a week. It looks like we won't get any feedback except the one which we got here from @rfscholte. Maybe it's time to complete with this pull request and with pull request #22 (these 2 pull request will produce merge conflicts which I resolved in merge/MEAR-153_into_MEAR-216 branch) and then release Maven EAR Plugin 3.2.0. Thank you for your help.

@rfscholte,

If you agree with my proposal and with my suggestion to implement the proposal in a dedicated (new) feature request (pull request), then please confirm that here (approval of this pull request is appreciated too). Please note that this pull request doesn't prevent implementation of that proposal in a backward compatible way - this is the thing which I care about, because I'd prefer to not implement skinnyModules feature at all if it will make impossible giving later the maximum control you suggested. Thank you for sharing this idea. I'll open Jira ticket for sharedLibs property of EAR module. Maybe I'll be able to work on it this year.

@mabrarov
Copy link
Contributor Author

@hboutemy,

I'm waiting for your feedback to merge this PR, then also to start a release

Let's start working on this if nobody has concerns.

@rfscholte
Copy link
Contributor

Looks good to me

@mabrarov
Copy link
Contributor Author

Hey guy, is it possible to release EAR plugin 3.2.0 with #24 and #22 this year? It would be a nice New Year's gift :) Thank you for your patience and work.

@rfscholte
Copy link
Contributor

Let's see what I can do, not sure if I have enough time to do it this year.

@mabrarov
Copy link
Contributor Author

mabrarov commented Dec 6, 2020

It looks like this pull request was partially merged, because the diff has reduced but is still not empty:

master...mabrarov:MEAR-153

screenshot1

@mabrarov
Copy link
Contributor Author

mabrarov commented Dec 6, 2020

I fixed that issue with not empty diff by merging the master branch into the source branch of this pull request. Now this pull request has empty diff.

@hboutemy, it looks like you merged source branch of this pull request into the master branch in 954b736, so I request you to take decision on this pull request - whether to merge this PR for correct history, or to decline this PR (because all changes were merged somehow without this PR).

@hboutemy
Copy link
Member

hboutemy commented Dec 6, 2020

Hi Marat,
yes, I merged this PR but amended the last commit message in 30395be because it was written against MEAR-216 instead of MEAR-153: I was waiting for Apache Jenkins job to finish before closing PR, Jira, and thanking you much for your hard work.

Did I miss something?

Regards,

Hervé

@mabrarov
Copy link
Contributor Author

mabrarov commented Dec 6, 2020

Hi @hboutemy,

Did I miss something?

No you didn't. It looks like you merged all changed I proposed in this PR. I'm just confused what to do with this PR.

@hboutemy
Copy link
Member

hboutemy commented Dec 6, 2020

It has been merged, even if GitHub can't really get it because of the last commit I amended...
let's close
and more importantly enjoy and say a big thank you for this nice new feature

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