-
Notifications
You must be signed in to change notification settings - Fork 40
Refactor Maven publication: maven -> maven-publish #398
Conversation
Codecov Report
@@ Coverage Diff @@
## master GoogleCloudPlatform/app-gradle-plugin#398 +/- ##
=========================================
Coverage 75.76% 75.76%
Complexity 277 277
=========================================
Files 42 42
Lines 1073 1073
Branches 41 41
=========================================
Hits 813 813
Misses 242 242
Partials 18 18 Continue to review full report at Codecov.
|
2e04e40
to
df0ab57
Compare
@loosebazooka , I believe the PR is ready. It generates the plugin markers and publishes them in one go (see sample https://oss.sonatype.org/content/repositories/comgithubvlsi-1131/ ) |
unfortunately, the workflow to do
is not viable given our signing process. |
What do you do for signing? |
Just in case, all the above options can be passed with environment variables and/or with gpg-agent. |
So our signing machine doesn't have internet access. The artifact is created -> copied to signing vm -> signed -> copied to another vm -> uploaded to sonatype. There's not much more I can say about the signing process. However, I'll play around with this and see if I can make it work. This is definitely an improvement that is useful to users. |
df0ab57
to
4c905c8
Compare
so yeah as long as you can generate all the necessary files into a known target directory, we can handle the followup work to publish it. |
4c905c8
to
0301023
Compare
What do you think of something like |
So again, thnks for your interest in doing this, and sorry that our release is a little bit of a blackbox. Right now what we do is generate a special folder "release-artifacts" that contains everything we need. This helps remove the logic of parsing the maven output from the release process. Could we do something similar to the current
|
Thanks for the interest! Can we retain the Groovy DSL syntax? Not that I don't like Kotlin, but I have zero knowledge with Kotlin and previously I had a lot of trouble fixing Gradle issues when dealing with Kotlin DSL (yeah, my problem). Even most of the team members don't have great expertise with Gradle, so I am afraid this may have adverse impact on us. |
I guess it can be done, however, it would require non-standard trickery. I am afraid it might be hard to maintain in the future. Are you sure the standard Maven layout does not work for you? What I mean is you would have to adapt your publishing anyway (e.g. hard-code
Ah. Let me see. I tried Groovy DSL, and it is hard to write for me as autocomplete does not really work there.
Well, that might depend on the IDE, however, I really got comfortable with Gradle only when I tried Kotlin DSL. |
0301023
to
b1af1a8
Compare
@loosebazooka , I've added |
Any blockers merging this? Having plugin markers available would make it easier to use the plugin. |
@martinbonnin This PR is blocked on reverting to Groovy DSL. |
I made a very crude first attempt at reverting to Groovy there: vlsi#1 @meltsufin any chance we can convince you to stick with Kotlin build scripts before reverting everything? Having autocomplete in the IDE is really nice. There are definitely downsides such as bigger compilation times but I find the developer experience much better. |
@chanseokoh Your call. |
@jamesward , please, https://twitter.com/_JamesWard/status/1472961164724289537 🙃 WDYT on #398 (comment) ? 😉 |
@vlsi the team has gained sufficient expertise with Kotlin, so we can actually accept the Kotlin migration. However, please keep the Groovy DSL in this PR. Fixing GoogleCloudPlatform/appengine-plugins#1004 and migrating to Kotlin at the same time gives hard time to reviewers, and if we ever have to revert either change, it is going to be a lot more work compared to filing two separate PRs. Big migration like this one has to be done in its own PR. |
Yay Kotlin! Glad to see that migration as the Gradle Kotlin DSL is so much nicer to work with than the Goovy DSL. Let me know if you need anything from me. |
@martinbonnin We're now just waiting for the PR to be split into two. |
@meltsufin, I see, thank you. I would split the PR unless Martin makes it faster. |
Go for it, I have a plentifull beginning of 2022! |
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.
The individual commits were very helpful, thank you.
I am going to have to update the internal release configuration to be able to test staging, but looking at what gets generated by the current script, I can work with it.
@@ -0,0 +1,16 @@ | |||
root = true |
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.
We don't typically check editorconfig into Java projects.
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.
Why's that?
editorconfig is helpful (e.g. "trim whitespace", "configure indentation", and even "import order for java"), so why do you suggest removing it?
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 isn't the hill I am planning to die on, but the answer is because nobody on the team that maintains this project uses editorconfig plugins.
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.
Do you use IntelliJ IDEA to edit the code, @elefeint? If so, you are using EditorConfig plugins: https://www.jetbrains.com/help/idea/editorconfig.html
Note that checking in this file would help external contributors (e.g. I edited the code, pressed autoformat shortcut, and EVERYTHING changed). We could have 0-configuration experience (import and it "just works"). Discovering and running gradlew googleJavaFormat
is just not the norm in the world of IDEs outside of Google.
build.gradle.kts
Outdated
id("com.github.sherter.google-java-format") version "0.8" | ||
id("checkstyle") | ||
id("jacoco") | ||
id("java") |
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.
Google's style guide is 2 spaces (there is no external Kotlin guide, but internal one says 2 spaces). More importantly, keeping unrelated style changes out of PRs makes the inevitable code archaeology so much easier. (The nice individual commits will get squashed on merge)
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.
Can you please point me to Google style guide for Kotlin that says "2 spaces"?
google/styleguide#314 says Google style is (see https://github.com/google/styleguide/pull/365/files) https://developer.android.com/kotlin/style-guide, which says "4 spaces" https://developer.android.com/kotlin/style-guide#indentation just like the Kotlin official coding conventions: https://kotlinlang.org/docs/coding-conventions.html#indentation
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.
@elefeint , would you please clarify if "squashing on merge" is an absolute requirement?
Squashing commits breaks git history analysis, and committing individual changes as separate commits helps that.
Do you suggest I should file 8 individual PRs?
The squash in #423 (comment) caused build.gradle.kts
to look like "a completely fresh file written by @martinbonnin" even though Martin tried their best to translate to Kotlin one-to-one.
More importantly, keeping unrelated style changes out of PRs makes the inevitable code archaeology so much easier
In fact, IntelliJ IDEA shows annotate for the "re-indented" lines just fine:
As you see, the indentation is new, however, "commit line" attribution is proper (it does not show all the lines as if they were modified by me).
GitHub UI is slightly different, and it does treat re-indent as commit that changes the line: https://github.com/vlsi/app-gradle-plugin/blame/refactor_publication/build.gradle.kts#L61, however, if the changes are NOT squashed, then there's a convenient "show changes prior the commit" button that shows the annotations prior to re-indent: https://github.com/vlsi/app-gradle-plugin/blame/2a49f56b03fa5c71343f829efd189debb3b9bcc3/build.gradle.kts
I incline that committing individual changes as separate commits helps for code archaeology
, and re-indenting as a separate commit does not make it much harder.
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.
Squashing is not an absolute requirement; merely a common org/project practice. Since your individual commits are well structured, I can rebase-merge this pull request specifically.
For the indentation, though, keeping 2 space indents would be ideal. We will work on externalizing Google's Kotlin style guide with @jamesward; in the meantime you'll have to take my word for it, I'm afraid.
6fe411b
to
0ca6b04
Compare
f0d4b4a
to
2be535f
Compare
@elefeint , could you please review the PR again? I've rebased the PR and I added a couple of changes:
|
@vlsi can you please address #398 (comment) GitHub PR changes look like a mess, but they don't have to be :) (I know there are individual commits, but the changes added together should also just address what it says on the tin (title), right?) Also since this is a major publishing change, you should compare the output of the published artifacts (e.g. same POM, same jar contents), tests passing might not be enough for making sure users are not breaking, or losing non-functional features (like browsing source code or javadoc, or getting the right licensing info from the POM). |
@vlsi I've abdicated my GCP review powers in favor of building a data analytics platform. |
@meltsufin any news here? This PR adds a lot of nice improvements and makes it easier for newcomers to consume the plugin, would be really nice to merge it. |
…Kotlin guide uses 2 spaces)
…ecks.xml when task is not executed
…et instead of string-based references
0dd1315
to
a8112e8
Compare
Gradle supplies the relevant API at the runtime, so there's no need for adding gradelApi as runtime dependency.
a8112e8
to
45cd6d2
Compare
I've realigned the indentation to 2 spaces, however, it would be nice to see the indentation size documented somewhere.
@TWiStErRob , I have no interest in splitting this PR into individual PRs as there's been virtually no feedback on the PR during the past couple of years. |
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.
I (an external contributor like you) reviewed the Gradle changes, they look good, this is mergeable if Kokoro can handle the directory change. I'll leave the .editorconfig up to the owners.
I like the small (safe) modernizations you did for the Gradle script. My comments are mostly small improvements to DX and maintainability. @vlsi feel free to do as you wish with them.
I tested the output artifacts, they look equivalent to current master
except the snapshot names, see comment on relevant line in prepareRelease
.
virtually no feedback on the PR during the past couple of years.
I feel you, it's strange that there are contributors working on stuff, but there's no activity from the owners (not just in this org/repo). While running prepareRelease
I noticed a lot of javadoc warnings, I was thinking I could fix those, and then I realized I already fixed them a few months ago in #452...
I have no interest in splitting this PR into individual PRs
I see now that it's not that many extra things, I think it was more in the past. I generally try to split different aspects into different PRs, because there's a higher chance of review when
- the PR is smaller (e.g.
tasks.jar.configure {
->tasks.jar {
-like changes in a separate PR is a trivial review and merge), - or less contentious (e.g. .editorconfig might block this PR, even though everything else could be merged).
- and both the above make the main changes more understandable and the PR smaller as well.
// Gradle will definitely supply the proper API at the runtime, so we should not | ||
// ask for anything else. | ||
compileOnly(gradleApi()) |
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 can be removed, see org.gradle.plugin.devel.plugins.JavaGradlePluginPlugin#applyDependencies
. It's already forcefully api
.
gradlePlugin { | ||
plugins { | ||
create("appengine") { | ||
id = "$group.appengine" |
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 much as it is duplicate code, I think it's better to have this inlined:
id = "$group.appengine" | |
id = "com.google.cloud.tools.appengine" |
so that text searches can find the ID on GitHub (or even Google) and even locally with grep
or in IDE.
description = | ||
"This Gradle plugin provides tasks to build and deploy Google App Engine applications." | ||
} | ||
create("appengine-appenginewebxml") { |
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.
These names become part of task names, having a dash inside them makes the tasks look very strange and unconventional:
create("appengine-appenginewebxml") { | |
create("appengineWebXml") { |
+
create("appengineAppYaml") {
etc.
Could even remove the word appengine
as it seems a bit redundant in this repo :)
(This is the only thing this "name" is used for as far as I know, not exposed anywhere in publication, so this is a safe devex change.)
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.
Frankly speaking, I would rather hide all these tasks from the default gradlew tasks output, however, it would increase PR surface which would reduce the likelihood of merging the PR
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.
Don't work against Gradle, work with it 😅. They're listed and actually useful if you just want to run that one task, and also to see what publications are there without having to touch the build scripts. These tasks helped me a lot while I was working on publishing multiple times.
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.
Note: having nicer names won't increase the complexity of the PR, but provide a better experience for anyone looking at tasks / reading build scripts.
compileClasspath += main.get().output | ||
runtimeClasspath += main.get().output | ||
} | ||
val integTestSourceSet = sourceSets.create("integTest") { |
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.
Oh, it would be nice to migrate this jvm-test-suite
, but this PR is required to be merged first so Gradle can be updated to latest first.
eachFile { | ||
path = "plugin-markers/$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.
Are you sure this flattening is required to separate the artifacts? They all have unique names.
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.
I’ve no idea on what are the requirements
@JoeWang1127 can you share more context why this was closed? |
Hi @martinbonnin, we plan to archive this repo and I see this PR hasn't been touched for a few months. The content of this repo now lives in https://github.com/GoogleCloudPlatform/appengine-plugins, feel free to open another one there. |
See GoogleCloudPlatform/appengine-plugins#1004
This uses Gradle's default means for generating plugin markers.
You can publish the artifacts to MavenLocal repository with
./gradlew publishToMavenLocal
As an alternative option, you can run
./gradlew publishToTmpMaven
to publish the files tobuild/repo
directory.=>
There's
./gradlew prepareRelease
task to mimic the previous behaviour:=>