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

Add maven profiles and caching to speed up builds. #1590

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

motlin
Copy link
Contributor

@motlin motlin commented May 21, 2024

I have two changes here:

  • Separating maven plugins into profiles and only enabling the relevant profiles in the relevant workflows
  • Adding the GitHub shared action for caching the .m2 directory

These are only related in that they both are intended to speed up the build and they both were created by comparing with another open source GitHub workflow which is written in this style.

The maven profile thing creates a third related change, splitting the "test" configuration into separate test, enforcer, and dependency:analyze-only jobs.

If this is too much, I can definitely split out the caching, and possibly more.

@motlin motlin force-pushed the maven-profiles-caching branch 3 times, most recently from 3e67bb2 to 77b2a83 Compare May 21, 2024 18:45
@motlin motlin marked this pull request as ready for review May 21, 2024 18:45
@motlin motlin marked this pull request as draft May 21, 2024 22:15
@motlin motlin force-pushed the maven-profiles-caching branch 3 times, most recently from ef16c7b to 2ad4ef3 Compare May 22, 2024 00:14
@@ -468,7 +386,7 @@
<includes>
<include>**/*Test.java</include>
</includes>
<argLine>-XX:-OmitStackTraceInFastThrow -Xms1024m -Xmx2048m @{argLine}</argLine>
<argLine>-XX:-OmitStackTraceInFastThrow -Xms1024m -Xmx2048m --add-opens java.base/java.lang=ALL-UNNAMED @{argLine}</argLine>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bit was a surprise to me. Jacoco must have been doing this on our behalf. Without this, unit tests and serialization tests started to fail in Java 17 and higher when calling clone().

@motlin
Copy link
Contributor Author

motlin commented May 22, 2024

Before:
Screenshot 2024-05-21 at 8 42 52 PM

After:

Screenshot 2024-05-21 at 8 43 02 PM

@motlin motlin marked this pull request as ready for review May 25, 2024 12:57
@motlin motlin force-pushed the maven-profiles-caching branch 2 times, most recently from d4ef310 to 731ecbd Compare May 28, 2024 14:12
@motlin motlin force-pushed the maven-profiles-caching branch 2 times, most recently from 01a1b0b to bfbbf49 Compare June 3, 2024 18:04
@motlin
Copy link
Contributor Author

motlin commented Jun 14, 2024

Could we please land this? I just resolved the merge conflicts between this and the OSGi stuff, and I have other branches stacked on top of this one.

@motlin motlin force-pushed the maven-profiles-caching branch 2 times, most recently from 5795ba0 to ac4ccca Compare June 14, 2024 13:13
@motlin
Copy link
Contributor Author

motlin commented Jun 14, 2024

@fipro78 I rebased my changes and now I'm getting build failures I don't understand, just in the Javadoc workflows. Could you help me figure them out?

13:14:44:117 [ERROR] /home/runner/work/eclipse-collections/eclipse-collections/eclipse-collections-api/target/generated-sources/java/org/eclipse/collections/api/factory/primitive/DoubleBooleanMaps.java:[25,21] error: package aQute.bnd.annotation.spi is not visible
  (package aQute.bnd.annotation.spi is declared in the unnamed module, but module aQute.bnd.annotation.spi does not read it)
13:14:44:117 [ERROR] /home/runner/work/eclipse-collections/eclipse-collections/eclipse-collections-api/target/generated-sources/java/org/eclipse/collections/api/factory/primitive/DoubleBooleanMaps.java:[26,21] error: package aQute.bnd.annotation.spi is not visible
  (package aQute.bnd.annotation.spi is declared in the unnamed module, but module aQute.bnd.annotation.spi does not read it)
...

@fipro78
Copy link
Contributor

fipro78 commented Jun 14, 2024

I also had issues with the javadoc builds while I worked on the pr.

One thing is that I had to build the generators first separately, as otherwise there are dependency issues.

The other thing is that the javadoc step needs to be done before the install.

Apart from that, I don't know the changes you are rebasing that could cause the breakage. But the two topics I mentioned above did cost me a while to fix.

Hope that helps.

@motlin
Copy link
Contributor Author

motlin commented Jun 14, 2024

The other thing is that the javadoc step needs to be done before the install.

The point of running install first is to make sure the code generator is built and all code-generated code is generated since it also has Javadoc. I'll try splitting the steps differently.

@motlin motlin force-pushed the maven-profiles-caching branch 5 times, most recently from 74cb501 to d955223 Compare June 15, 2024 02:56
@motlin motlin force-pushed the maven-profiles-caching branch 4 times, most recently from c626d8a to e39741f Compare June 16, 2024 03:46
Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

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

@motlin What do the region Phase numbers mean?

@motlin
Copy link
Contributor Author

motlin commented Jun 16, 2024

@motlin What do the region Phase numbers mean?

Even after using maven all this time, I get confused about what order the plugins will run in. I got in the habit of numbering by phases.

https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#lifecycle-reference

I start with validate is 1 and just count from there. It's not a common convention but I find it helpful.

@donraab
Copy link
Contributor

donraab commented Jun 16, 2024

@motlin What do the region Phase numbers mean?

Even after using maven all this time, I get confused about what order the plugins will run in. I got in the habit of numbering by phases.

https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#lifecycle-reference

I start with validate is 1 and just count from there. It's not a common convention but I find it helpful.

I found them a bit confusing because there were references to phase 16, phase 17, but no other phases in some files.

Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

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

@motlin I am approving and you can feel free to merge. @fipro78 if you can validate there are no issues with your recent changes, I would appreciate it. Thanks!

@motlin motlin merged commit 5f85f80 into eclipse:master Jun 17, 2024
18 checks passed
@motlin motlin deleted the maven-profiles-caching branch June 17, 2024 13:28
@fipro78
Copy link
Contributor

fipro78 commented Jun 17, 2024

As far as I can see this PR "only" improves the build execution. Only in the sense of "it should not affect my recent OSGi and JPMS changes". From having a look at the changes sources, I would not see any effect in that area.

To be 100% sure you should double check the generated artefacts. Open the generated jar files for API and IMPL and ensure that the META-INF/MANIFEST.MF contains now quite a lot of information, there is a module-info.class file on top level and the META-INF/services in the IMPL jar file contain the factory services definitions.

@motlin
Copy link
Contributor Author

motlin commented Jun 17, 2024

While I was able to land this by working in GitHub workflows, I find I'm no longer able to do development for Eclipse Collections in the IDE, or run certain maven builds. I consistently get errors like this.

java: package aQute.bnd.annotation.spi is not visible
  (package aQute.bnd.annotation.spi is declared in the unnamed module, but module org.eclipse.collections.api does not read it)

@fipro78
Copy link
Contributor

fipro78 commented Jun 17, 2024

Maybe the same issue as on github? Is a multi stage build required locally?

I developed my PR in Eclipse and got it working there. But I am not sure about the intermediate steps. It was a longer journey.

@donraab
Copy link
Contributor

donraab commented Jun 18, 2024

@motlin @fipro78 Looking at the diff, a large section was removed from the parent pom in this PR. I will build in IDE to see if I see same issues.

Craig, I can confirm I am having the same issues building and running in the IDE. Additionally, there is a dependency issue with koloboke impl jar when running analyze.

@donraab
Copy link
Contributor

donraab commented Jun 19, 2024

@motlin I was able to get IntelliJ to work running Unit Tests and running Unit Tests with Coverage by disabling the IntelliJ build and run in the configuration for All Unit Tests. I changed the IntelliJ JUnit configuration to use run only, after running Maven clean/test.

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.

None yet

3 participants