-
Notifications
You must be signed in to change notification settings - Fork 475
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
base: master
Are you sure you want to change the base?
Conversation
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 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 |
/signed |
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! |
/reviewers 2 |
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. |
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. |
Same. I think that we should look at the functionality it needs to provide and use that as the baseline for what it contains.
Same, which is why I'm inclined to give JavaFX itself a chance to revise its publishing code.
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?
If the build.gradle can be broken up even more (based on the functionalities) it could help making heads and tails of it. |
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. |
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 In short, the change is:
Now that we have the publication based on the Gradle Model (by setting it up with
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. |
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. |
There was a problem hiding this 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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
testImplementation project(":base").sourceSets.test.output | ||
implementation project(':base') | ||
api project(':base') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
archiveBaseName = moduleName | ||
archiveClassifier = t.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
@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! |
Keep open. |
@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! |
I'd definitely like to see this getting merged. |
❗ This change is not yet ready to be integrated. |
There are 2 open issues with this PR (actually 3, as it needs to have a corresponding JBS issue but that is easily solved)
Maybe the fix to the first issue (moving the maven publication bits out of the build.gradle) might help with the second? |
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:
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. |
From my perspective there are probably 3 different topics in the last two comments:
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. |
Yes, these are several different issues. I was just outlining a plan to decompose the larger build file.
Did the instructions at https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX not work for you?
I can test on Windows and Ubuntu.
This will require either Johan or Kevin I think |
f11dbb6
to
8825da6
Compare
@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:
But as there is no single machine that can cross-build this, and 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:
To create the metadata correctly |
@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! |
@jjohannes this pull request can not be integrated into 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 |
@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! |
What is blocking this issue at this point? @jjohannes Are you waiting for explanations/help regarding the publishing part? |
@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...
|
I don't know how the publishing step works. @johanvos or @kevinrushforth should. I tried to split the |
@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
@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! |
@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! |
@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! |
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. |
For publishing to my local .m2 repository, I do
@tiainen can you share the gradle command we do for publishing to maven central? |
We do the publication in two steps:
|
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. |
Maybe I did not express the question clearly enough. If I do this locally:
I get this:
But how do I get the Jars for the other operating systems? Is the command above somehow called several times? |
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:
|
@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? |
In more technical detail, this is the process we follow to get the artifacts published into maven central.
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 |
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
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