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

Gradle catalog + platform #268

Merged
merged 21 commits into from
Jun 11, 2024
Merged

Gradle catalog + platform #268

merged 21 commits into from
Jun 11, 2024

Conversation

elect86
Copy link
Contributor

@elect86 elect86 commented Apr 25, 2024

Gradle Catalog and Platform properly generated and the corresponding artifacts get added alongside the main publication

We'd still need to verify whether this version does matter or less, we'll see with a snapshot publications maybe?

Another cool thing Gradle Platform can do is aligning to boms as well, like here

api(platform("com.fasterxml.jackson:jackson-bom:" + 
    libs.com.fasterxml.jackson.core.jacksonCore.get().version))

But we need to coordinate this, there are other boms which may be interesting, such as jakarta.jakartaee-bom and tons of google ones, which is a mess for someone like me which is not familiar..

@elect86 elect86 assigned ctrueden and unassigned ctrueden Apr 25, 2024
@elect86 elect86 requested a review from ctrueden April 25, 2024 20:37
@elect86 elect86 changed the title Gradle catalog + platform Gradle catalog + platform + declarative enforcer plugin Apr 26, 2024
@elect86
Copy link
Contributor Author

elect86 commented Apr 26, 2024

Just pushed a working sketch of plugins to enforce a pure declarative Gradle build script

plugins under declarative, all the different test under declarative-playground

It's based on the current wip of Gradle

@elect86
Copy link
Contributor Author

elect86 commented May 7, 2024

so, together with @jjohannes, we clean up the project the best possible

  • one single build generating both catalog and platform
  • moved .build under target/gradle
  • removed all files we could, including .gitignore, gradle.properties and gradle.bat

the only build directory isn't possible to move is .gradle

I tested mvn install and looks fine

.gitignore Outdated
@@ -1,3 +1,4 @@
/.project
/.settings/
/target/
/gradle-scijava/.gradle

Choose a reason for hiding this comment

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

@elect86 I would add /.idea/ here and remove all the .idea files this PR adds right now.

pom.xml Outdated
<execution>
<id>attach-gradle-catalog</id>
<!-- Check if correct-->
<phase>package</phase>

Choose a reason for hiding this comment

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

I think it works without adding the <phase> here at all. The default should be fine. See similar setup here:
https://github.com/google/guava/blob/master/guava/pom.xml#L205

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, I can confirm it works

@ctrueden
Copy link
Member

ctrueden commented May 8, 2024

@elect86 Thank you for continuing to work on this! And @jjohannes thank you for reviewing!

Here is the current error happening on CI:

java.io.IOException: Cannot run program "./gradlew" (in directory "/home/runner/work/pom-scijava/pom-scijava/target/gradle-scijava"): error=2, No such file or directory

With 5a514ac I tried to be very explicit about the intended working directory, but even then the same error persists. So more investigation is needed.

@elect86 elect86 changed the title Gradle catalog + platform + declarative enforcer plugin Gradle catalog + platform May 8, 2024
@elect86
Copy link
Contributor Author

elect86 commented May 8, 2024

I have no idea why the CI is so problematic...
locally mvn install works absolutely fine with ./gradlew on linux..

@elect86
Copy link
Contributor Author

elect86 commented May 8, 2024

with an ls it work fine ./gradlew... wth

@elect86
Copy link
Contributor Author

elect86 commented May 8, 2024

So, here Maven runs Gradle just fine, we can see it start downloading the distro

And here the task is executed

> Task :generateCatalogAndPlatform

However, after a while the task gets executed once again here

and a third time here

and then the crash happens after a while here

@elect86
Copy link
Contributor Author

elect86 commented May 15, 2024

SO post

@elect86
Copy link
Contributor Author

elect86 commented Jun 7, 2024

The problem lies somewhere in the complexity of the CI

here and here

@ctrueden
Copy link
Member

@elect86 and I spoke earlier today about the issue maybe being the mavenEvaluate function calls, which invoke mvn ... exec:exec. The purpose of mavenEvaluate is not to actually run the build, but rather only evaluate expressions, typically to get the values of Maven properties and so forth. The build script uses it to obtain the SCM URL for the project, as well as the project version, and version of the maven-gpg-plugin (to decide whether a particular build hack is needed).

While it does seem like quite a coincidence that the Gradle-specific logic is triggered multiple times, and there are three "extra" invocations of mvn in the ci-build.sh script, the mavenEvaluate function is not actually the culprit. On the CI, we see the message:

  --> maven-gpg-plugin version OK: 3.1.0

which gets printed after all three of the mavenEvaluate calls happen... but it not until the main build (== Building the artifact locally only ==) that the Gradle logic is invoked for the first time. And that time it works, including installation of the gradle-generated artifacts into the local Maven repository cache.

No, the issue is that the Gradle platform/catalog logic happens again as part of the integration testing, a.k.a. the "mega melt": a subsequent step to ensure that all component versions work together as advertised. To reproduce the problem locally, you can invoke tests/run.sh from a pom-scijava working copy.

Specifically, the script performs the following (slightly simplified) commands to set up a temporary doctored pom-scijava parent for use with local testing:

mkdir -p target
cp pom.xml target/pom.xml
mvn -B -f target/pom.xml versions:set -DnewVersion=999-mega-melt
mvn -B -f target/pom.xml install

It seems Maven sets the CWD and ${basedir} to match the directory of the pom.xml file passed via the -f flag.

To fix, I changed the mvn ... install line, which was running through the Maven lifecycle including the attempted gradle catalog+platform generation, to mvn ... install:install, which runs only the install goal itself, which in the case of the mega-melt is all that's needed: we just need the special 999-mega-melt version of the POM to be available locally as a parent for the individual components that will be rebuilt. 9ce8ddd

Once I fixed that initial issue, there still remained another problem, however. The pom-scijava POM is intended as the parent POM for downstream components. As currently written, any project extending this updated version of pom-scijava inherits the exec-maven-plugin execution that tries to launch gradle. We do not want this; the gradle execution is supposed to occur only for the pom-scijava build itself, not builds of downstream components that extend it. I implemented a workaround with 9cd9000, which wraps the gradle catalog+platform generation into a profile that gets activated only when a scijava-gradle directory is present in the project basedir. That way, when building pom-scijava itself, the gradle build runs, and when using pom-scijava as a parent in other projects, it does not (unless they happen to have a file called scijava-gradle in their basedir, which is highly unlikely).

@elect86
Copy link
Contributor Author

elect86 commented Jun 11, 2024

Awesome, thanks for taking care of this

So now, can it be merged? Can we test it with a snapshot publication?

elect86 and others added 8 commits June 11, 2024 10:55
For some reason, the CI attempts to run ./gradlew from
the directory target/gradle-scijava, rather than gradle-scijava.
Let's prepend the ${basedir} to be more explicit about what we want.
Signed-off-by: Curtis Rueden <[email protected]>
We do not need to perform the entire Maven lifecycle just to install the
doctored 999-mega-melt version of the pom-scijava parent into the local
repository cache. We can just invoke the install:install goal directly.

Not only does this speed up the mega-melt slightly (e.g. no enforcer
checks), it also avoids an issue whereby the Gradle platform+catalog
build would fail due to being in the wrong working/base directory.
Let's leave it to consumers to decide which metadata model is "richer"
and which one they "should prefer". They know their own use cases.
We don't want it being triggered in downstream projects that
extend pom-scijava -- only in the pom-scijava build itself.
@ctrueden ctrueden merged commit 030a64e into master Jun 11, 2024
1 check passed
@ctrueden ctrueden deleted the gradlePlatform branch June 11, 2024 16:25
@elect86
Copy link
Contributor Author

elect86 commented Jun 11, 2024

thanks dear

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.

3 participants