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

Publish Gradle Metadata with variants and attributes #1232

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjohannes
Copy link

@jjohannes jjohannes commented Sep 5, 2023

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:

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

  • 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

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Sep 5, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 5, 2023

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.

@jjohannes
Copy link
Author

/signed

@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Sep 5, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 5, 2023

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!

@nlisker
Copy link
Collaborator

nlisker commented Sep 5, 2023

/reviewers 2

@openjdk
Copy link

openjdk bot commented Sep 5, 2023

@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).

@kevinrushforth
Copy link
Member

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?

@nlisker
Copy link
Collaborator

nlisker commented Sep 5, 2023

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 gradlex-org/jvm-dependency-conflict-resolution#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.

@johanvos
Copy link
Collaborator

johanvos commented Sep 7, 2023

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 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.

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.

@nlisker
Copy link
Collaborator

nlisker commented Sep 7, 2023

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?

@bridgekeeper bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Sep 7, 2023
@johanvos
Copy link
Collaborator

johanvos commented Sep 7, 2023

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.

@jjohannes
Copy link
Author

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 openjfx/javafx-gradle-plugin#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.

@kevinrushforth
Copy link
Member

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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Actually, I took a look at the changes, and this will affect more than just the maven publishing task. I left a few questions about that. If the other changes really are needed, then it will need a lot more testing.

@@ -2057,7 +2072,7 @@ allprojects {

// All of our projects are java projects

apply plugin: "java"
apply plugin: 'java-library'
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this change? This affects more than just the maven publishing, so I would like to make sure there are no implications to the rest of our build.

Copy link
Author

Choose a reason for hiding this comment

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

As I explained in #1232 (comment) this changes the setup so that Gradle automatically generates the metadata (pom and module files) based on the "Model of the Software". Before the change, the metadata was completely constructed manually.

This change is needed in this PR to then extend the Model with the "Platform Variants" so that these are then also added to the metadata. (But it is in general a good improvement to let Gradle generate the metadata instead of to maintaining custom code for that).

In short you can say, all these changes pull things up one abstraction level. Instead of directly creating all metadata in the publications {} block, we give the information to Gradle in the "right place" so that it then has a complete and correct model of the JavaFX libraries and there relationships. From which it then creates the metadata when publishing.

This particular change here is needed, because since Gradle 4+, "java-library" is used to represent Java libraries. Which all of the JavaFX components are in Gradle terminology. This make the "api" scope for dependencies available we need further down.

Comment on lines 2266 to +2288
testImplementation project(":base").sourceSets.test.output
implementation project(':base')
api project(':base')
Copy link
Member

Choose a reason for hiding this comment

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

Same question here. This seems unrelated to the changes you are trying to make.

Copy link
Author

Choose a reason for hiding this comment

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

This is correcting the dependency scopes to be like they are in the pom after publishing. Because the information is correct here now, we no longer need the lists of dependency modules in these calls: addMavenPublication(project, [ 'base' ]) (in this example, the [base] is removed here).

Comment on lines +5016 to +5046
archiveBaseName = moduleName
archiveClassifier = t.name
Copy link
Member

Choose a reason for hiding this comment

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

Same question here.

Copy link
Author

@jjohannes jjohannes Sep 9, 2023

Choose a reason for hiding this comment

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

Pulling this up from the publications {} block (where this information is removed) into the "Model of the Software".

See also the ! part of the documentation here: https://docs.gradle.org/current/userguide/publishing_customization.html#sec:publishing_custom_artifacts_to_maven

build.gradle Outdated
@@ -1719,16 +1718,38 @@ void addMavenPublication(Project project, List<String> projectDependencies) {
}

compileTargets { t ->
// TODO split out the correct 'os' and 'arch' values from 't.name'
def os = t.name
def arch = "x86"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This TODO is something that needs to be done in this PR, right? How would this work together with the platform-specific gradle files we have in buildSrc?

Copy link
Author

Choose a reason for hiding this comment

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

This should have no influence on how the platform-specific gradle files we have in buildSrc work. This is for determining how the different platform variants are identified by attributes in the metadata. Gradle has predefined attributes and values for OPERATING_SYSTEM and ARCHITECTURE.

If my understanding is correct, in a publishing build this code block will be used for each "JavaFX platform". And t represents the current platform. What I don't know if how to best map t to OPERATING_SYSTEM and ARCHITECTURE. t is of type:

class CompileTarget {
    String name;
    String upper;
    String capital;
}

So it only has a "name" and IIUC from that name we can somehow get to the OPERATING_SYSTEM and ARCHITECtURE – as the "name" is also used to set the classifier of the Jar.

For reference, and clarification, here is the relationship between JavaFXPlatforms and Gradle attribute values in the JavaFX plugin:
https://github.com/openjfx/javafx-gradle-plugin/blob/master/src/main/java/org/openjfx/gradle/JavaFXPlatform.java#L42-L46

    LINUX("linux", "linux-x86_64", OperatingSystemFamily.LINUX, MachineArchitecture.X86_64),
    LINUX_AARCH64("linux-aarch64", "linux-aarch_64", OperatingSystemFamily.LINUX, MachineArchitecture.ARM64),
    WINDOWS("win", "windows-x86_64", OperatingSystemFamily.WINDOWS, MachineArchitecture.X86_64),
    OSX("mac", "osx-x86_64", OperatingSystemFamily.MACOS, MachineArchitecture.X86_64),
    OSX_AARCH64("mac-aarch64", "osx-aarch_64", OperatingSystemFamily.MACOS, MachineArchitecture.ARM64);

We can add a similar list here, but I was wondering if the information is somewhere in the build already (?) And the list above also seems to be incomplete as there are also the monocle variants that are not in there.

@MarceloRuiz
Copy link

MarceloRuiz commented Nov 2, 2023

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.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 28, 2023

@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!

@nlisker
Copy link
Collaborator

nlisker commented Dec 28, 2023

Keep open.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 22, 2024

@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!

@sschuberth
Copy link

sschuberth commented Feb 22, 2024

I'd definitely like to see this getting merged.

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

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

@johanvos
Copy link
Collaborator

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?

@nlisker
Copy link
Collaborator

nlisker commented Mar 21, 2024

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.

@jjohannes
Copy link
Author

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.

@nlisker
Copy link
Collaborator

nlisker commented Mar 22, 2024

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

@jjohannes
Copy link
Author

@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.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 19, 2024

@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!

@openjdk
Copy link

openjdk bot commented Jun 25, 2024

@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 openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jun 25, 2024
@bridgekeeper
Copy link

bridgekeeper bot commented Aug 20, 2024

@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!

@nlisker
Copy link
Collaborator

nlisker commented Aug 20, 2024

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

@jjohannes
Copy link
Author

@nlisker yes. As I explained in the comment above, this solution is not finished. Initially, I did not understand that the build runs several times for publishing (one time for each of the published platforms). At least that what I pieced together from the information I found.

Can you point me at the CI setup (or scripts) that do the actual publishing to Maven Central? I would like to...

  1. fully understand how this works right now
  2. figure out a way to reproduce this locally (publishing to a local folder), so that I can test this PR and extend it with what is missing

@nlisker
Copy link
Collaborator

nlisker commented Sep 2, 2024

I don't know how the publishing step works. @johanvos or @kevinrushforth should.

I tried to split the base module out of the big build file and hit the publishing code as well, so I stopped until this is resolved. The base module, being relatively simple (few dependencies), is a good starting point for decluttering.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 28, 2024

@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!

3 similar comments
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 30, 2024

@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
Copy link

bridgekeeper bot commented Oct 31, 2024

@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
Copy link

bridgekeeper bot commented Oct 31, 2024

@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!

@Vampire
Copy link

Vampire commented Oct 31, 2024

Hi @Vampire, 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 Vampire" 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.

@johanvos
Copy link
Collaborator

johanvos commented Nov 17, 2024

@nlisker yes. As I explained in the comment above, this solution is not finished. Initially, I did not understand that the build runs several times for publishing (one time for each of the published platforms). At least that what I pieced together from the information I found.

Can you point me at the CI setup (or scripts) that do the actual publishing to Maven Central? I would like to...

  1. fully understand how this works right now
  2. figure out a way to reproduce this locally (publishing to a local folder), so that I can test this PR and extend it with what is missing

For publishing to my local .m2 repository, I do

sh gradlew -PMAVEN_PUBLISH=true sdk publishToMavenLocal

@tiainen can you share the gradle command we do for publishing to maven central?

@tiainen
Copy link
Collaborator

tiainen commented Nov 18, 2024

We do the publication in two steps:

  • we first build the maven jars to be published with the command already listed above: sh gradlew ... -PMAVEN_PUBLISH=true all publishToMavenLocal
  • the actual publication is done separately from the regular OpenJFX build by a custom process in our CI infrastructure

@johanvos
Copy link
Collaborator

We do the publication in two steps:

  • we first build the maven jars to be published with the command already listed above: sh gradlew ... -PMAVEN_PUBLISH=true all publishToMavenLocal
  • the actual publication is done separately from the regular OpenJFX build by a custom process in our CI infrastructure

That actually means that there is currently no standard publication process to publish artifacts to an external repository in the build.gradle. I also think this should not be part of the build.gradle itself, as this is something that distributions should deal with. And even if it would be part of openjfx, I really think this does not belong in the same file that contains all the logic of e.g. building native webkit code for different platforms.
I'm all open to discuss this, but I think this issue is separate from the discussion about splitting the build.gradle into smaller, more maintainable files.

@jjohannes
Copy link
Author

Maybe I did not express the question clearly enough. If I do this locally:

sh gradlew -PMAVEN_PUBLISH=true all publishToMavenLocal

I get this:

javafx-base
├── 24-internal+0-2024-11-18-123143
│   ├── javafx-base-24-internal+0-2024-11-18-123143-mac.jar
│   ├── javafx-base-24-internal+0-2024-11-18-123143.jar
│   └── javafx-base-24-internal+0-2024-11-18-123143.pom
└── maven-metadata-local.xml

But how do I get the Jars for the other operating systems? Is the command above somehow called several times?
IIUC, this must be part of the configuration in CI? Can you share that part of the configuration?

@johanvos
Copy link
Collaborator

johanvos commented Nov 18, 2024

But how do I get the Jars for the other operating systems? Is the command above somehow called several times? IIUC, this must be part of the configuration in CI? Can you share that part of the configuration?

That is indeed true: the build process (that is, the build.gradle file) which is already very complex has nothing to do with the publication process. The build.gradle is not used to upload the artifacts to maven central or other repositories (afaik). And I don't think it should -- in hindsight, adding some maven publication logic inside the build.gradle was a mistake I made that I wish I could revert (ha, rereading the whole thread and I already said this before -- broken record again, sorry ;) ).

I believe everyone who produces artifacts based on openjfx does that in a complex CI setup, and I'm all +1 in standardizing this process, but it should imho not be part of the build process. But again, this is not done using the build.gradle or any other file in OpenJFX. In our (Gluon) case, this is part of a Jenkins job with a bunch of jenkins scripts that start sub-jobs on expensive AWS slave instances where each platform-specific artifact is built, and then the resulting jars are combined and upload in one chunk to maven central, after which some smoke tests are automatically done and if successful, the repository is closed and published. The process of combining the jars and uploading them does not uses the build.gradle. (at least, that is how I believe it works, @tiainen can correct me when I'm wrong).

Hence, I believe we should separate the 2 discussions:

  1. Gradle Metadata -- which is the original question in this PR, and which should not touch build.gradle apart from removing functionality (from build.gradle) that can then go in a separate script/tool/...

  2. maintaining the build.gradle file, and reducing the complexity, which is not the scope of this PR.

@jjohannes
Copy link
Author

@johanvos I am fine with putting the publishing configuration, and everything we might add to that in this PR if we continue, into a separate file.

Independent of where you put things: As asked above, for me to take a look at this again, I need to reproduce the full publishing (or at least publishing for more than one OS) locally. For that, I would like some more information about how this is done right now. Do you run the build on different machines (e.g. Linux and Windows agents) and then merge the results somehow? Do you publish individual parts from different machines somehow? Or do you use docker? Or can you cross-compile everything somehow?
I would really appreciate if you could share the corresponding prat of the CI config for that. Otherwise it is difficult for me to update this PR so that it proposes a complete working solution.

@tiainen
Copy link
Collaborator

tiainen commented Nov 21, 2024

In more technical detail, this is the process we follow to get the artifacts published into maven central.

  1. We start the build jenkins job to create a build for a specific tag in the repository.
  2. A target jenkins job is started for each platform that needs to be built. We add the property -PMAVEN_PUBLISH=true to the gradle command to enable the publishToMavenLocal gradle task. At the end of the target job, the generated maven artifacts are archived so that it can be downloaded again from the build job.
  3. When all target jobs have completed, the build job takes over to handle the actual maven publication.
  4. Run the gradle build to generate the javadoc and gather the java sources:
./gradlew --no-daemon --info --refresh-dependencies \
  -PCONF=Release \
  -PMILESTONE_FCS=${env.MILESTONE_FCS}\
  -PPROMOTED_BUILD_NUMBER=${env.PROMOTED_BUILD_NUMBER} \
  -PHUDSON_BUILD_NUMBER=${env.BUILD_NUMBER} \
  -PHUDSON_JOB_NAME=${env.JOB_BASE_NAME} \
  -PUSE_DEPEND=false \
  copySourceFilesLinux javadoc
  1. Create a folder from where we publish to maven central: mkdir -p mavenpublication/${env.PROMOTED_BUILD_NUMBER}. The promoted build number is a value that we derive from the git tag and is the part after the + sign. E.g. for the tag 23+29, the promoted build number is 29.
  2. Copy all necessary files into that folder:
cp -r build/javadoc mavenpublication/${env.PROMOTED_BUILD_NUMBER}/
cp -r build/modular-sdk/modules_src mavenpublication/${env.PROMOTED_BUILD_NUMBER}/
cp -r gradle mavenpublication/
cp gradlew mavenpublication/gradlew
cp gradlew.bat mavenpublication/gradlew.bat
cp build.properties mavenpublication/build.properties
cp tools/maven/build.gradle mavenpublication/
cp tools/maven/javafx.pom mavenpublication/
  1. Enter the directory and do some more preparation:
cd mavenpublication
chmod +x gradlew
touch settings.gradle
  1. Copy the archived maven artifacts from each target job for every platform into the folder ${env.PROMOTED_BUILD_NUMBER}. We also generate a variable called PUBLICATION_TARGETS that will contain a comma separated list of all targets that needs publishing. This will be used by the gradle build to determine which targets are going to be published. The values are the actual classifiers that we use for each target: linux, linux-aarch64, mac, mac-aarch64 and win.
  2. Run the gradle command to publish the artifacts:
./gradlew --no-daemon --info --refresh-dependencies \
  -Dorg.gradle.internal.repository.max.tentatives=5 \
  -Dorg.gradle.internal.repository.initial.backoff=500 \
  -Dorg.gradle.internal.publish.checksums.insecure=true \
  -Dorg.gradle.internal.http.connectionTimeout=60000 \
  -Dorg.gradle.internal.http.socketTimeout=60000 \
  -PCONF=Release \
  -PPUBLICATION_TARGETS=${PUBLICATION_TARGETS} \
  -PMILESTONE_FCS=${MILESTONE_FCS} \
  -PMAVEN_PUBLISH=true \
  -PPROMOTED_BUILD_NUMBER=${PROMOTED_BUILD_NUMBER} \
  -PHUDSON_BUILD_NUMBER=${BUILD_NUMBER} \
  -PHUDSON_JOB_NAME=${JOB_BASE_NAME} \
  -PUSE_DEPEND=false \
  -PBUILD_SRC_ZIP=true \
  -PCOMPILE_WEBKIT=true \
  -PCOMPILE_MEDIA=true \
  -PBUILD_LIBAV_STUBS=true \
  -PSTUB_RUNTIME= \
  -PrepositoryUrl=${MAVEN_REPOSITORY_URL} \
  -PrepositoryUsername=${MAVEN_REPOSITORY_USERNAME} \
  -PrepositoryPassword=${MAVEN_REPOSITORY_PASSWORD} \
  -Psigning.secretKeyRingFile=${MAVEN_SIGNING_KEY_FILE} \
  -Psigning.password=${MAVEN_SIGNING_PASSWORD} \
  -Psigning.keyId=${MAVEN_SIGNING_KEY_ID} \
  clean publish

Here is a gist that contains our build.gradle together with the pom file that we use for the publication: https://gist.github.com/tiainen/e451d1538969c1f827ae878a3a8a368d
https://gist.github.com/tiainen/e451d1538969c1f827ae878a3a8a368d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict Pull request has merge conflict with target branch
Development

Successfully merging this pull request may close these issues.

8 participants