8333146: Move maven publication logic from build.gradle

[Maran23]

This PR splits the maven publishing logic into an own file, maven-publish.gradle, that is next to the root build.gradle.

The build.gradle will apply the maven-publish.gradle. The maven-publish.gradle will then configure the Maven related properties and register all modules for publication.

This way, we decoupled the logic that much, the only things we need to do:

• apply from maven-publish.gradle
• Call configureMavenPublication later in the build chain.

To better understand the context, this is the commit where the whole Maven Publishing logic was introduced. I moved all of that logic out of the main build.gradle: 5a18677f

This will reduce the size by ~170 lines for the build.gradle.

Tested with:

• ./gradlew -PMAVEN_PUBLISH=true -PMAVEN_VERSION=custom publishToMavenLocal
• ./gradlew -PMAVEN_PUBLISH=true publishToMavenLocal
• ./gradlew -PMAVEN_PUBLISH=true -PMILESTONE_FCS=true publishToMavenLocal

Everything still works: -> Example: javafx.base from the local .m2 repository

I think this is a good step and an easy way to split out functionality without blowing things up. We might want to do that for other parts as well.

Note: I also fixed the deprecated buildDir, the deprecated project.task method and 2 warnings where it seems like he might not be able to infer the type (changing def to the actual type). -> The file is completely green, no warnings or deprecations. Note2: I did a small improvement to the addMavenPublication method. It is now more 'typesafe', e.g. projects must be passed in directly, not as String. If a project does not exist, the build will fail with an exception.

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8333146: Move maven publication logic from build.gradle (Enhancement - P4)

Reviewers

• Joeri Sykora (@tiainen - Author) 🔄 Re-review required (review applies to 1af96337)
• Johan Vos (@johanvos - Reviewer) 🔄 Re-review required (review applies to ea0b427c)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1970/head:pull/1970
$ git checkout pull/1970

Update a local copy of the PR:
$ git checkout pull/1970
$ git pull https://git.openjdk.org/jfx.git pull/1970/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1970

View PR using the GUI difftool:
$ git pr show -t 1970

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1970.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

[mlbridge[bot]]

Webrevs

• 05: Full - Incremental (74a9d1f6)
• 04: Full - Incremental (6d8e7282)
• 03: Full - Incremental (ea0b427c)
• 02: Full - Incremental (b465f5e0)
• 01: Full - Incremental (2b45aa63)
• 00: Full (1af96337)

[kevinrushforth]

Reviewers: @johanvos @tiainen and one of @kevinrushforth or @arapte

/reviewers 2 reviewers

[openjdk[bot]]

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[nlisker]

Note that there's a relevant stale PR in https://github.com/openjdk/jfx/pull/1232 by @jjohannes. Maybe he'd like to take a look.

[Maran23]

Note that there's a relevant stale PR in #1232 by @jjohannes. Maybe he'd like to take a look.

Thanks. It looks like my work will not block/interfere the implementation of that PR, which is good. It should be easy to pick the changes from there over in the new maven-publish.gradle. Should also help for reviewing, since it is more isolated. :-)

[jjohannes]

Thanks for the ping. I have to rework my PR anyway. There is no need to pay special attention to it. Any change that makes the publishing setup clearer is good IMO. I think this PR is a good step forward.

(I didn't get back to the topic yet on the my PR, because it is more work as I initially expected. What is complicating things me there is how conceptually get the setup right with the Jars for different platform built on different machines. I have to set aside some mote time for it.)

[Maran23]

I found a way to further decouple the maven publishing logic!

Now we only need to: apply from: 'maven-publish.gradle' after all projects were defined. Later in the build chain, we need to call the configureMavenPublication Closure from 'maven-publish.gradle', and we are done.

It will work like before, so there is no logical change. But still need a re-review. I retested all configurations.

[kevinrushforth]

It will work like before, so there is no logical change. But still need a re-review. I retested all configurations.

Correction: It isn't just a re-review that is pending. This PR requires review from @johanvos and @arapte (or me, but I'll defer to Ambarish).

[johanvos]

@tiainen reviewed it and is ok with it. I had a quick look, and it doesn't cause regression. Also note that we're making progress with the build-openjfx-using-openjdk project, and we hope to create our builds with that approach in the near future.

[Maran23]

Correction: It isn't just a re-review that is pending.

Indeed! My bad, I didn't express myself clearly. What I meant was that everyone who has already checked this needs to check it again, as some things have changed (not just minor things like the copyright).

[Maran23]

@tiainen reviewed it and is ok with it. I had a quick look, and it doesn't cause regression. Also note that we're making progress with the build-openjfx-using-openjdk project, and we hope to create our builds with that approach in the near future.

That sounds very promising. Good to hear!

[arapte]

Could you please verify this observations:

1. With this change the jar files are generated without class files i.e. sdk is not built. Use command : gradle -PMAVEN_PUBLISH=true publishToMavenLocal. Note that default task sdk is not specified in the command. The command completes in just a few seconds (~20 seconds), whereas the sdk task can take a few minutes Attaching the screenshot, please observe that size of jar files are just a few 100 bytes. They should be in KBs.

2. If sdk task is added to the above command as : gradle -PMAVEN_PUBLISH=true publishToMavenLocal sdk then the build fails with below error:

> Task :base:buildModuleMac FAILED

[Incubating] Problems report is available at: build/reports/problems/problems-report.html

FAILURE: Build failed with an exception.

* What went wrong:
A problem was found with the configuration of task ':base:buildModuleMac' (type 'Copy').
  - Gradle detected a problem with the following location: 'modules/javafx.base/build/module-classes'.
    
    Reason: Task ':base:modularPublicationJarMac' uses this output of task ':base:buildModuleMac' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
    
    Possible solutions:
      1. Declare task ':base:buildModuleMac' as an input of ':base:modularPublicationJarMac'.
      2. Declare an explicit dependency on ':base:buildModuleMac' from ':base:modularPublicationJarMac' using Task#dependsOn.
      3. Declare an explicit dependency on ':base:buildModuleMac' from ':base:modularPublicationJarMac' using Task#mustRunAfter.
    
    For more information, please refer to https://docs.gradle.org/9.2.0/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.

* Try:
> Declare task ':base:buildModuleMac' as an input of ':base:modularPublicationJarMac'
> Declare an explicit dependency on ':base:buildModuleMac' from ':base:modularPublicationJarMac' using Task#dependsOn
> Declare an explicit dependency on ':base:buildModuleMac' from ':base:modularPublicationJarMac' using Task#mustRunAfter
> Run with --scan to generate a Build Scan (powered by Develocity).

Deprecated Gradle features were used in this build, making it incompatible with Gradle 10.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/9.2.0/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD FAILED in 1m 15s

[Maran23]

Could you please verify this observations:

1. With this change the jar files are generated without class files i.e. sdk is not built. Use command : gradle -PMAVEN_PUBLISH=true publishToMavenLocal. Note that default task sdk is not specified in the command. The command completes in just a few seconds (~20 seconds), whereas the sdk task can take a few minutes Attaching the screenshot, please observe that size of jar files are just a few 100 bytes. They should be in KBs.

Thanks. I can reproduce this! I always built the sdk first and then run the publishing. But you are right, this should work together as well. My guess is that the maven task need to depend on the build task, that should fix both problems. Will do some testing and report back.

[Maran23]

Both issues should be fixed now. It really was just a matter of depending the maven publication task on the buildModulesTask, which makes the most sense to me.

[arapte]

I ran the same command gradle -PMAVEN_PUBLISH=true publishToMavenLocal with the latest changes in the PR and without.

With this PR, for a module a few additional tasks get executed compared to without PR. For example for graphics module, following additional tasks are executed.

> Task :graphics:modularJarStandaloneMac
> Task :graphics:copyNativeFilesStandaloneMac NO-SOURCE
> Task :graphics:copyLibFilesStandaloneMac
> Task :graphics:copyLegalStandaloneMac
> Task :graphics:cleanOpenLegalStandaloneMac
> Task :graphics:copyClassFilesMac
> Task :graphics:copyBinFilesMac NO-SOURCE
> Task :graphics:copyLibFilesMac
> Task :graphics:copySourceFilesMac
> Task :graphics:copyDocFilesMac
> Task :graphics:copyBuildPropertiesMac
> Task :graphics:copyClosedLegalStandaloneMac
> Task :graphics:copyLegalMac

So, the content of build directories differ. With this change there are 2 additional directories created under build dir: modular-sdk and sdk

I am not sure which is more correct behavior or if it can cause any issue. Though seems safe, as ideal way would be to do a full build and then publish. I would like to request @kevinrushforth and @johanvos to please weigh in.

[Maran23]

So, the content of build directories differ. With this change there are 2 additional directories created under build dir: modular-sdk and sdk I am not sure which is more correct behavior or if it can cause any issue. Though seems safe, as ideal way would be to do a full build and then publish. I would like to request @kevinrushforth and @johanvos to please weigh in.

Thanks a lot for testing as well! I saw that too and thought the same, it should be more safe and makes more sense. Before, it was depending on each individual module build task like so: https://github.com/openjdk/jfx/blob/f87448ec156608527d77a4204e98e08052ffecd1/build.gradle#L5850 Which I think is much more error prone and also risks that we forget something or it is executed too early in the future.

[kevinrushforth]

@Maran23 I agree that the change @arapte noted above is fine, and your explanation makes sense.

I did find one other difference in artifacts created.

With the current master, running gradle sdk creates build/publications (because the sdk task indirectly depends on the tasks that create the artifacts for publication). With this patch, it doesn't. That might be an OK change.

However, I also note that with this patch, even gradle all doesn't create build/publications. Consider adding a dependency to either gradle sdk or, more likely, gradle all so that still creates build/publications.

[Maran23]

However, I also note that with this patch, even gradle all doesn't create build/publications. Consider adding a dependency to either gradle sdk or, more likely, gradle all so that still creates build/publications.

Thanks for testing! Just so we have a common understanding: Depending on the all task seems to be a better choice than sdk? I guess because then we are sure that everything did run before, right?

[kevinrushforth]

However, I also note that with this patch, even gradle all doesn't create build/publications. Consider adding a dependency to either gradle sdk or, more likely, gradle all so that still creates build/publications.

Thanks for testing! Just so we have a common understanding: Depending on the all task seems to be a better choice than sdk? I guess because then we are sure that everything did run before, right?

Either is fine, but having the all task depend on it seems a better choice (the fact that the sdk task currently depends on it seems somewhat arbitrary).

[Maran23]

Either is fine, but having the all task depend on it seems a better choice (the fact that the sdk task currently depends on it seems somewhat arbitrary).

I had a look and gave it a shot.

If we depend on the all task, we will now also build all apps, which I think is not a good thing. Since this is unexpected if we just want to (build and) publish the Maven Libraries. In my opinion, the sdk task makes more sense (or a task that will do everything except the apps).

[kevinrushforth]

Either is fine, but having the all task depend on it seems a better choice (the fact that the sdk task currently depends on it seems somewhat arbitrary).

I had a look and gave it a shot.

If we depend on the all task, we will now also build all apps, which I think is not a good thing. Since this is unexpected if we just want to (build and) publish the Maven Libraries. In my opinion, the sdk task makes more sense (or a task that will do everything except the apps).

I think you might have it backwards: all should depend on the task to build the publications, not the other way around. The goal is that gradle all should produce build/publications.

[Maran23]

I think you might have it backwards: all should depend on the task to build the publications, not the other way around. The goal is that gradle all should produce build/publications.

Ah okay, I got you now! I will have a look how this can be achieved.

[Maran23]

I think you might have it backwards: all should depend on the task to build the publications, not the other way around. The goal is that gradle all should produce build/publications.

Did you have a specific idea in mind when writing the message? I ran a few tests, but nothing worked without explicitly writing publishToMavenLocal within the command. And I don't want to worsen the separation between the main and publish gradle files. If there's no idea at the moment, then I'll continue looking for a good solution!

EDIT: Also, -PMAVEN_PUBLISH=true need to be specified always as of now. Again, this is because it is completely separated now (and don't run if it is set to false, which will save some seconds for the normal case where this is not needed). I don't have a good idea for this right now.

[Maran23]

I think you might have it backwards: all should depend on the task to build the publications, not the other way around. The goal is that gradle all should produce build/publications.

Here is an attempt to do that without coupling the build with maven-publish: https://github.com/Maran23/jfx/commit/d325d9931b37c64641fb9064e5deadbfa5766fe8

This probably needs to be done with compileTargets then. But the idea is the same.