Associate (possibly empty) build output dir (target/classes) with upstream projects rather than local m2 repository #1570

scottkurz · 2022-08-03T02:18:42Z

…ile because compilation wasn't done

Signed-off-by: Scott Kurz skurz@us.ibm.com

Fixes #1569 (or at least starts the effort).

scottkurz · 2022-08-03T02:32:11Z

This approach simply tries to adjust for the fact that the CompilerMojo happens to not associate the "target/classes" dir with the dependency Artifact in the special case that there is no source to compile.... causing core Maven resolver function to resolve this Artifact against the local repo, (leading to the fix in #1207 we're now revisiting).

Not sure of the downside in my approach. Presumably there may be some (or if not we might imagine proposing a change in CompilerMojo , though my guess is the community wouldn't think it worth the risk). But within the more narrow dev mode usage it seems like adding our own custom behavior could be worth it.

I did try to understand why a multi-module mvn compile doesn't have the same exact issue as #1201. If I debugged correctly, it seems the reason is there is a special case in https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/ReactorReader.java via which, if we have a 'compile' phase, we will resolve to the output dir somehow. (IIUC).

TODO

Is the 'classes' in 'target/classes' configurable? I'm not sure it is? I see in the maven-compiler doc the config for multi release builds. Not sure if we support them in dev mode.
I suspect this test will fail IT since it looks like the change in Purge locally installed upstream modules when using loose app #1207 tests the implementation detail of purging from the local repo rather than just testing the original Dev mode does not show errors if upstream module already installed in .m2 #1201 external. We'd need to rework the tests.
I see the run goal has a purge as well. I haven't looked at this.

kathrynkodama · 2022-08-03T21:08:32Z

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/DevMojo.java

@@ -1173,15 +1175,22 @@ protected void doExecute() throws Exception {
                if (isEar) {
                    runMojo("org.apache.maven.plugins", "maven-ear-plugin", "generate-application-xml");
                    runMojo("org.apache.maven.plugins", "maven-resources-plugin", "resources");
-
                    installEmptyEarIfNotFound(project);
                } else if (project.getPackaging().equals("pom")) {
                    log.debug("Skipping compile/resources on module with pom packaging type");
                } else {
                    purgeLocalRepositoryArtifact();


Why do we still need to purge the local repository?

Whoops... bad rebase... I fixed it. Let me test again though , when I get a chance, to make sure I didn't mess this up.

Maybe I could take this to the next step, which I forgot to mention and make sure the watcher compile fails as well.

By make sure the watcher compile fails as well, do you mean making sure the watcher compile has target/classes in its artifacts list to resolve against?

That's right. I updated the PR for this just now.

Thx for taking a quick look. Sorry for the formatting changes that I see snuck in here.

kathrynkodama

Looks good to me, thank you @scottkurz (and thanks for fixing the formatting)

Should we disable this test for now - https://github.com/OpenLiberty/ci.maven/blob/main/liberty-maven-plugin/src/it/dev-it/src/test/java/net/wasdev/wlp/test/dev/it/MultiModuleM2InstalledTest.java (just add @Ignore) and we can rework it in a follow up issue

scottkurz · 2022-08-05T19:01:30Z

Looks good to me, thank you @scottkurz (and thanks for fixing the formatting)

Should we disable this test for now - https://github.com/OpenLiberty/ci.maven/blob/main/liberty-maven-plugin/src/it/dev-it/src/test/java/net/wasdev/wlp/test/dev/it/MultiModuleM2InstalledTest.java (just add @Ignore) and we can rework it in a follow up issue

Thanks for taking a look. We could just disable the test ....
But we should at least look at the run goal before merging.

scottkurz · 2022-08-09T18:41:18Z

@kathrynkodama A note on the inability of the logs to capture the failing output.

There's a problem with a pattern like:

assertTrue(getLogTail(logFile), <condition> , timeout));

We're going to do the getLogTail() immediately, but there's no log yet.. the separate mvn liberty:dev process is just starting up.

It looks like this has been addressed in JUnit 5 by adding a Supplier to defer message calculation.
https://stackoverflow.com/a/63921478/1499766

I at least added a failure TestWatcher..... which might help a bit locally (the test afterXXX cleans up the logs) but won't help in say the GHA environment.

Signed-off-by: Scott Kurz <skurz@us.ibm.com>

kathrynkodama · 2022-08-10T19:47:37Z

liberty-maven-plugin/src/main/java/io/openliberty/tools/maven/server/PluginConfigSupport.java

+        File outputDir = Paths.get(mavenProject.getBuild().getOutputDirectory()).toFile();
+        artifactToUpdate.setFile(outputDir);
+        try {
+            outputDir.createNewFile();


Any concern with creating an output directory for modules without any src/main/java code, ie. for ear modules? I can't think of any downside to doing this and just having the empty directory...

scottkurz · 2022-08-10T22:44:29Z

@kathrynkodama your comment jogged my memory that we were doing the install empty EAR thing.

So I went back and made a similar change in that path, so we can programmatically conjure up a "workspace" (i.e. reactor) EAR artifact without having to install one. So we can kind of avoid touching the local .m2 repo altogether it would seem.

I added a path to use 'target' rather than 'target/classes' in case that is a bit less weird.

I left the purge/install code in, just in case we need to resort to it in the case that there are some file permission issues with creating directories.

So this ends up fixing #1203 and providing an alternate solution to #1176

…ile because compilation wasn't done Signed-off-by: Scott Kurz <skurz@us.ibm.com>

Signed-off-by: Scott Kurz <skurz@us.ibm.com>

kathrynkodama reviewed Aug 3, 2022

View reviewed changes

scottkurz force-pushed the add-upstream-File-on-skipped-compilation branch 4 times, most recently from 2ceb61f to e534881 Compare August 4, 2022 20:40

scottkurz changed the title ~~Add target/classes File to upstream multi-module Artifact missing a F…~~ Associate (possibly empty) build output dir (target/classes) with upstream projects rather than local m2 repository Aug 4, 2022

kathrynkodama approved these changes Aug 5, 2022

View reviewed changes

scottkurz force-pushed the add-upstream-File-on-skipped-compilation branch 4 times, most recently from 27b34b6 to bf660a5 Compare August 9, 2022 18:37

formatting

f60a555

Signed-off-by: Scott Kurz <skurz@us.ibm.com>

scottkurz force-pushed the add-upstream-File-on-skipped-compilation branch from bf660a5 to a73572a Compare August 10, 2022 13:15

kathrynkodama mentioned this pull request Aug 10, 2022

Update the Integration Tests to wait for dev mode to start completely #1554

Open

scottkurz force-pushed the add-upstream-File-on-skipped-compilation branch 2 times, most recently from 4893028 to b59866b Compare August 10, 2022 15:43

kathrynkodama reviewed Aug 10, 2022

View reviewed changes

scottkurz force-pushed the add-upstream-File-on-skipped-compilation branch 4 times, most recently from fc94069 to 9bf566d Compare August 10, 2022 22:43

scottkurz added 2 commits August 10, 2022 19:02

Add target/classes File to upstream multi-module Artifact missing a F…

43be982

…ile because compilation wasn't done Signed-off-by: Scott Kurz <skurz@us.ibm.com>

Adjust ITs for new multi-mod behavior

a8870cf

Signed-off-by: Scott Kurz <skurz@us.ibm.com>

scottkurz force-pushed the add-upstream-File-on-skipped-compilation branch from 9bf566d to a8870cf Compare August 10, 2022 23:03

kathrynkodama approved these changes Aug 11, 2022

View reviewed changes

scottkurz requested a review from TrevCraw August 11, 2022 17:51

TrevCraw approved these changes Aug 12, 2022

View reviewed changes

scottkurz merged commit b9e4546 into OpenLiberty:main Aug 12, 2022

This was referenced Aug 12, 2022

Dev mode does not show errors if upstream module already installed in .m2 #1201

Closed

Dev goal relies on upstream module dependencies to exist #1203

Closed

Dev mode multi module - automate workaround to install ear project to .m2 #1176

Closed

scottkurz mentioned this pull request Sep 8, 2022

Multi-module liberty dev mode fails when there exists a test-jar dependency #1561

Closed

scottkurz mentioned this pull request Nov 1, 2023

Issues with copyDependencies of artifact built within multi-module reactor build #1752

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Associate (possibly empty) build output dir (target/classes) with upstream projects rather than local m2 repository #1570

Associate (possibly empty) build output dir (target/classes) with upstream projects rather than local m2 repository #1570

scottkurz commented Aug 3, 2022

scottkurz commented Aug 3, 2022 •

edited

Loading

kathrynkodama Aug 3, 2022

scottkurz Aug 3, 2022

scottkurz Aug 4, 2022

kathrynkodama Aug 4, 2022

scottkurz Aug 4, 2022

scottkurz Aug 4, 2022

kathrynkodama left a comment •

edited

Loading

scottkurz commented Aug 5, 2022

scottkurz commented Aug 9, 2022

kathrynkodama Aug 10, 2022

scottkurz commented Aug 10, 2022 •

edited

Loading

Associate (possibly empty) build output dir (target/classes) with upstream projects rather than local m2 repository #1570

Associate (possibly empty) build output dir (target/classes) with upstream projects rather than local m2 repository #1570

Conversation

scottkurz commented Aug 3, 2022

scottkurz commented Aug 3, 2022 • edited Loading

TODO

kathrynkodama Aug 3, 2022

Choose a reason for hiding this comment

scottkurz Aug 3, 2022

Choose a reason for hiding this comment

scottkurz Aug 4, 2022

Choose a reason for hiding this comment

kathrynkodama Aug 4, 2022

Choose a reason for hiding this comment

scottkurz Aug 4, 2022

Choose a reason for hiding this comment

scottkurz Aug 4, 2022

Choose a reason for hiding this comment

kathrynkodama left a comment • edited Loading

Choose a reason for hiding this comment

scottkurz commented Aug 5, 2022

scottkurz commented Aug 9, 2022

kathrynkodama Aug 10, 2022

Choose a reason for hiding this comment

scottkurz commented Aug 10, 2022 • edited Loading

scottkurz commented Aug 3, 2022 •

edited

Loading

kathrynkodama left a comment •

edited

Loading

scottkurz commented Aug 10, 2022 •

edited

Loading