jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Publish Gradle Metadata with variants and attributes

Open jjohannes opened this issue 2 years ago • 48 comments
trafficstars

This would make it possible to use JavaFX from Gradle by setting attributes for platform selection on the classpaths without using an additional plugin.

For explanation and discussions leading up to this, see:

  • https://github.com/openjfx/javafx-gradle-plugin/pull/151

Note: this is not yet tested and there is one important TODO left.

If you are interested in to adopt this, maybe someone can point me at how a complete publication is built. From looking at the setup, my understanding is that first the different platforms are "built" (on different machines?) and then then all the results are combined in a separate publishing run (with COMPILE_TARGETS to "all") and published. If that is true, could I get hold a build result for all platforms to then test the publishing changes with that? (Or does it work completely different? 😃 )


Progress

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1232

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

Using diff file

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

jjohannes avatar Sep 05 '23 12:09 jjohannes

Hi @jjohannes, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jjohannes" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

bridgekeeper[bot] avatar Sep 05 '23 12:09 bridgekeeper[bot]

/signed

jjohannes avatar Sep 05 '23 12:09 jjohannes

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

bridgekeeper[bot] avatar Sep 05 '23 12:09 bridgekeeper[bot]

/reviewers 2

nlisker avatar Sep 05 '23 12:09 nlisker

@nlisker 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 1 Reviewer, 1 Author).

openjdk[bot] avatar Sep 05 '23 12:09 openjdk[bot]

I note that we are very unlikely to accept this PR. Similar proposals have been made in the past, and we are not planning to expand the Maven publishing capabilities of the JavaFX build.

@johanvos Do you want to say more?

kevinrushforth avatar Sep 05 '23 12:09 kevinrushforth

I note that we are very unlikely to accept this PR. Similar proposals have been made in the past, and we are not planning to expand the Maven publishing capabilities of the JavaFX build.

@johanvos Do you want to say more?

This is a continuation of a short discussion on https://github.com/gradlex-org/java-ecosystem-capabilities/issues/57.

The way I see it, this is more of a fix than an enhancement. There is an old discussion on the mailing list where the conclusion, from what I understand, was that the fix should have been made in Gradle. I believe that has happened, and now JavaFX should update itself to use this update from Gradle. The current way we do it (with the empty jars) is somewhat sketchy. The external JavaFX plugin was developed to fill in the shortcomings of the current mechanism, but if we do publishing "properly" I don't think we will need it, though it depends on the flexibility of use cases.

I also think that the publishing parts of the build.gradle file should be split into their own file (publishing.gradle) since they are not part of the build, but this is a different discussion.

nlisker avatar Sep 05 '23 13:09 nlisker

I believe both @kevinrushforth and @nlisker have correct and relevant points. I believe the "different discussion" as mentioned by Nir is actually the first discussion though. We currently have a mix of building and deploying/distributing functionality in one (way too) big file and that is not how it should be imho. In hindsight, I regret adding the maven publication rules in the build.gradle. Adding more gradle-specific code would make it worse. As a rule of thumb, I'd love to see PR's where the amount of code in build.gradle is decreasing, not increasing.

I am very impressed by the achievement of @jjohannes in https://github.com/openjfx/javafx-gradle-plugin/pull/151 and the result is that the javafx gradle plugin is now easier to use and more in line with the current gradle development. That's extremely helpful.

I'm a bit hesitating to go one step further and do this already now in OpenJFX, as the Gradle functionality/api/best practices are subject to change, and I'd like the OpenJFX repository to focus on building the code -- less in distributing it.

Having separate files for the publication process (both for maven and gradle) make sense as it doesn't make the build.gradle more complex or dependent on changing defaults/guideline in both the gradle and maven ecosystems. I'm not sure those files belong in the openjdk/jfx repository at all, but I'm open for discussions about this.

johanvos avatar Sep 07 '23 11:09 johanvos

As a rule of thumb, I'd love to see PR's where the amount of code in build.gradle is decreasing, not increasing.

Same. I think that we should look at the functionality it needs to provide and use that as the baseline for what it contains.

I am very impressed by the achievement of @jjohannes in openjfx/javafx-gradle-plugin#151 and the result is that the javafx gradle plugin is now easier to use and more in line with the current gradle development. That's extremely helpful.

Same, which is why I'm inclined to give JavaFX itself a chance to revise its publishing code.

I'm a bit hesitating to go one step further and do this already now in OpenJFX, as the Gradle functionality/api/best practices are subject to change, and I'd like the OpenJFX repository to focus on building the code -- less in distributing it.

Understandable, I often hit issues with these changes too. I don't follow the evolution of the gradle ecosystem closely. Do the proposed changes rely on very new code whose best practices didn't settle in yet? Are these changes disruptive to backwards compatibility?

Having separate files for the publication process (both for maven and gradle) make sense as it doesn't make the build.gradle more complex or dependent on changing defaults/guideline in both the gradle and maven ecosystems. I'm not sure those files belong in the openjdk/jfx repository at all, but I'm open for discussions about this.

If the build.gradle can be broken up even more (based on the functionalities) it could help making heads and tails of it. As for where to put the publishing files, where does the OpenJDK JDK put them? What other options are there, a different repository?

nlisker avatar Sep 07 '23 14:09 nlisker

If the build.gradle can be broken up even more (based on the functionalities) it could help making heads and tails of it. As for where to put the publishing files, where does the OpenJDK JDK put them? What other options are there, a different repository?

The JDK doesn't have this "problem" as there are no maven/gradle artifacts to be created -- what it builds goes into the JDK and does not need to be loaded at build/compile time. It's just building jmods and native libs etc. There are a number of options (e.g. build a distribution/image, build static libs,...) and those are the result of a nice configure/make system where you pass a number of options to configure and you can run some targets with make.

johanvos avatar Sep 07 '23 15:09 johanvos

Thanks you all for the quick and detailed responses and discussions. Let me share a bit more on what this change does and my thoughts on it.

From my perspective, extending the metadata for Gradle users and reworking the build (splitting up the files, moving it somewhere else, ...) are two separate topics that would make sense to be discusses (and decided on) separately.

In this PR, I purposefully did not want to do something with the complexity of the build.gradle file (not making it better, but also not making it worse). An indication of this is that the PR roughly removes as many lines as it adds. Nevertheless, it does one improvement: The change uses Gradle's standard publishing mechanisms to generate the metadata files (.pom and .module) instead of the complete custom pom metadata generation it did before.

In short, the change is:

  • We us from project.components.java to configure the publication. By this, Gradle takes the "Model" of the Java projects (which Jars and which dependencies make up the components) and "serializes" that into metadata during publishing.
  • By this we can remove most of the custom metadata generation (removed projectDependencies.each { dep -> ...} code)
    • We still keep the custom "parent pom+profiles" part for the POM metadata, because that is ONLY FOR MAVEN users, and I think it's smart as it is the best you can do to support the multiple Platforms in Maven.
  • Because Gradle now automatically generates the metadata from the dependencies between the components, we remove explicit dependency lists for metadata generation – e.g. addMavenPublication(project, [ 'controls' ]) -> addMavenPublication(project)). Instead, we correct the dependencies between the components to be exactly like they should be in the metadata – .e.g implementation project(':base') -> api project(':base') (api corresponds to compile scope in Maven).

Now that we have the publication based on the Gradle Model (by setting it up with from components.java), we can declare all Platforms as separate variants – the one new config block containing the variants.create.... This information is then present in the Gradle Metadata for Gradle users. They can then select JavaFX Jars based on "Attributes". Even if they do not use the JavaFX Gradle plugin. For example:

// Configure the runtime classpath to select a certain platform variant for all Jars that have a native code aspects (JavaFX and others)
configurations.runtimeClasspath {
  attributes {
    attribute(OperatingSystemFamily.OPERATING_SYSTEM_ATTRIBUTE, objects.named(LINUX))
    attribute(MachineArchitecture.ARCHITECTURE_ATTRIBUTE, objects.named("x86-64"))
  }
}

As I mentioned in https://github.com/openjfx/javafx-gradle-plugin/pull/151, this mechanism is by now adopted in the wider JVM ecosystem, where native code is part of many libraries (Android, Kotlin-Multiplatform). These platforms have been a driver for adding these "variant" capabilities to Gradle's dependency management in the first place. I think it would be only natural for Java libraries that are at the heart of the ecosystem and also have a "native code aspect" adopt this as well.

Keeping the general "complexity of this build.gradle" issue aside: I think this would be a great improvement for all Gradle users that want to work with JavaFX. And I would be happy to get this done. I would just need some advice on how to test this.

Of course I also understand if now is not the right time to do this. Then you can put this aside and maybe get back to this later. I just would love to see this done eventually as I am convinced that we can only improve the general "dependency hell" situation in the Java ecosystem if all libraries publish more/better metadata. And Gradle now gives the opportunity to do that.


Regarding the issue of the complexity of the build: I think the state it is in is not surprising as the inital setup was done with an early Gradle version (Gradle 2.x?). By now, many things that this build would profit from are available in Gradle directly. I think much more of the setup can be replaced with standard Gradle solutions. The configuration could also be moved somewhere else and published as a "convention" plugin if you want to reduce what's in this repo. This is all supported by Gradle 7 or 8. My daily work at the moment is to help projects with such updates/migrations. I could give some advice and help (as my time allows) if you want to tackle this at some point. Still, it would be great if the decision on whether or not to publish Gradle Metadata could be made independent of this.

jjohannes avatar Sep 07 '23 18:09 jjohannes

The JDK doesn't have this "problem" as there are no maven/gradle artifacts to be created -- what it builds goes into the JDK and does not need to be loaded at build/compile time. It's just building jmods and native libs etc.

But it does have similar things that are needed, such as code signing and notarization, packaging up the final artifacts into packages or installers, and various other things that produce the distribution. And JavaFX also does similar things. Many of these are in build scripts / make files / etc that are not in the repo.

In hindsight, perhaps the Maven publishing scripts should not have been in the repo, as Johan says, but as long as it is, maybe it does make sense to modify it to add this meta data.

Ultimately, this is Johan's call. I have no opinion one way or the other.

kevinrushforth avatar Sep 08 '23 19:09 kevinrushforth

Hi @MarceloRuiz, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user MarceloRuiz for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

MarceloRuiz avatar Nov 02 '23 01:11 MarceloRuiz

@jjohannes This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Dec 28 '23 04:12 bridgekeeper[bot]

Keep open.

nlisker avatar Dec 28 '23 14:12 nlisker

@jjohannes This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Feb 22 '24 15:02 bridgekeeper[bot]

I'd definitely like to see this getting merged.

sschuberth avatar Feb 22 '24 15:02 sschuberth

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

openjdk[bot] avatar Mar 13 '24 20:03 openjdk[bot]

There are 2 open issues with this PR (actually 3, as it needs to have a corresponding JBS issue but that is easily solved)

  • the changes related to the metadata/variants are a good idea, but I prefer to have those in a separate file. I hope we can make th build.gradle smaller, not bigger.
  • it introduces more changes that need to be tested extremely well. Almost everything we do in OpenJFX depends on this single monolithical build.gradle file, so in general, I'm extremely careful not to change a single byte unless I know this has no impact on e.g. passing correct parameters to the webkit compilation.

Maybe the fix to the first issue (moving the maven publication bits out of the build.gradle) might help with the second?

johanvos avatar Mar 21 '24 08:03 johanvos

the changes related to the metadata/variants are a good idea, but I prefer to have those in a separate file. I hope we can make the build.gradle smaller, not bigger.

I have been using convention plugins to separate the needs for each subproject. It's like using interfaces in Java, where each class (Gradle subproject) implements interfaces (declares Gralde plugins). This splits the monolithic build file into behaviors that can compose together in each sub project's own build file.

Here are some of the tasks I would like to promote:

  • Create a convention plugin project, perhaps under the existing gradle folder (where the wrapper is) with the common parts that sub projects share. I see that the buildSrc folder contains some groovy scripts, but I'm not sure what they are for. Generally buildSrc is an "old relic" convention that isn't required anymore.
  • Split the publishing/deploying/distributing stuff out into its own file(s).
  • Reorganize the tasks both in terms of hierarchy ("relies on") and in terms of usefulness. Running builds and tests with and without webkit should be one command each, same for running system and robot tests, without the developer needing to remember the syntax and parameters. This will also speed up work as only the necessary commands are run instead of rerunning the full command because there's no finer-tuned one.
  • Create a file with the constants (like paths) that will be used in the build and script files. Finding where the build file searches for things and where it outputs them should be easy, especially for debugging work.

Let's remember that we are relying on @jjohannes's free time to help with this. This is not the situation where a regular contributor is more flexible with the tasks, so I think that we should take the help while we can by increasing the priority here. It might mean that we need many small and faster PRs under a larger umbrella issue. I can probably do the split into convention plugins myself, having done it a few times before.

nlisker avatar Mar 21 '24 16:03 nlisker

From my perspective there are probably 3 different topics in the last two comments:

  1. Add the Gradle Metadata Publishing with minimal changes to the existing build.gradle . This would be, as I see it, this PR with the addition of addressing this TODO and moving out the additions into a separate Gradle file. And then testing the result.
  2. Splitting up build.gradle into multiple "convention plugins"
  3. Reducing the complexity and size of the custom Gradle code by using more standard solutions the latest Gradle version offers.

I can offer to look at this PR again to solve (1). As I wrote in the original description, I would need help with testing as I did not figure out how to run the complete build (for the multiple platforms) myself. But maybe I can prepare this PR to the point where I think it should work and someone of you can do the testing. Maybe you can then create a snapshot that I can then check to verify that the metadata is as expected. I think this can be done independent of any other changes to the build, so that you will have this feature without tying it to a "larger" build refactoring.

For (2) and (3), it may make sense to do them together or in two steps. Or first do (3) and then (2). If you plan to tackle this @nlisker I can offer to review your work and give feedback and suggestions. But I would do that as a separate topic detached from this PR.

jjohannes avatar Mar 22 '24 08:03 jjohannes

For (2) and (3), it may make sense to do them together or in two steps. Or first do (3) and then (2). If you plan to tackle this @nlisker I can offer to review your work and give feedback and suggestions. But I would do that as a separate topic detached from this PR.

Yes, these are several different issues. I was just outlining a plan to decompose the larger build file.

I would need help with testing as I did not figure out how to run the complete build (for the multiple platforms) myself.

Did the instructions at https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX not work for you?

But maybe I can prepare this PR to the point where I think it should work and someone of you can do the testing.

I can test on Windows and Ubuntu.

Maybe you can then create a snapshot that I can then check to verify that the metadata is as expected.

This will require either Johan or Kevin I think

nlisker avatar Mar 22 '24 15:03 nlisker

@nlisker thanks for the pointers. I got a bit further to building and testing the publishing and updated this PR accordingly. I tested locally for one Compile Targets with metadata which looks good now.

The information on https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX is fine, but what I meant by "run the complete build" is running the publishing of all Compile Targets. So that in the end I have a repository (for testing this can be a folder on disk) with all the Compile Targets and the complete metadata.

Something like this:

$ ./gradlew publish -PMAVEN_PUBLISH=true -PCOMPILE_TARGETS=win,mac,linux,linux-aarch64,mac-aarch64

But as there is no single machine that can cross-build this, and linux-aarch64 and mac-aarch64 do not even exist as separate targets, I assume there is some "trickery" involved in building it all that consists of multiple Gradle calls on multiple machines. I would like to understand how it is done.

In this PR, when we create the metadata, we somehow nee to pretend that we build everything at once. Because all Jars should end up in the metadata. Right now there is this place in the PR where the variants are configured:

compileTargets { t ->
        def os = t.name
        def arch, classifierSuffix
        ...

To create the metadata correctly compileTargets would need to contain all of win,mac,linux,linux-aarch64,mac-aarch64 in a single Gradle call which is never the case. This needs to be tweaked, which I can do once I understand how the complete publishing process works.

jjohannes avatar Apr 24 '24 13:04 jjohannes

@jjohannes This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jun 19 '24 18:06 bridgekeeper[bot]

@jjohannes this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout publish-gradle-metadata
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Jun 25 '24 12:06 openjdk[bot]

@jjohannes This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Aug 20 '24 12:08 bridgekeeper[bot]

What is blocking this issue at this point? @jjohannes Are you waiting for explanations/help regarding the publishing part?

nlisker avatar Aug 20 '24 15:08 nlisker