Only configure publishing if it's applied externally #32351

alpar-t · 2018-07-25T05:52:39Z

Alternate implementation for #32195. As discussed there, we should only configure publishing if already applied.

elasticmachine · 2018-07-26T11:43:10Z

Pinging @elastic/es-core-infra

rjernst

A couple questions.

rjernst · 2018-07-30T23:59:33Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

+                    .parse(project.tasks.generatePomFileForNebulaPublication.outputs.files.singleFile)
+            pom.artifactId[0].value = project.name + "-client"
+            jarFile.resolveSibling(clientPomFileName).toFile().text = XmlUtil.serialize(pom)
+                    // Groovy has an odd way of formatting the XML, fix it up


Is this new? It's unclear why this wasn't needed before.

Probably the pom generation is Java and doesn't use this ?
We used to generate a new pom, now we copy and edit an existing one with this groovy code.
It shouldn't be necessary, as XML doesn't care about these, but I wanted to make sure it also looks as the previous one.

rjernst · 2018-07-31T00:01:32Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

-                        root.appendNode('description', project.pluginProperties.extension.description)
-                        root.appendNode('url', urlFromOrigin(project.scminfo.origin))
-                        Node scmNode = root.appendNode('scm')
-                        scmNode.appendNode('url', project.scminfo.origin)


Are you sure this isn't needed anymore? IIRC I added it because the scm info didn't get into the pom correctly at the time, and caused validation to fail when publishing to maven central.

I do remember doing diffs of before and after XMLs and didn't notice anything odd. Any edge cases or additional tests I should do ?

I just checked this again to make sure:

[:~/work/elastic/elasticsearch/modules/reindex] fix-multi-jar-publishing-take-2+ ± diff ./build/distributions/reindex-client-7.0.0-alpha1-SNAPSHOT.pom ~/tmp/reindex-client-7.0.0-alpha1-SNAPSHOT.pom.master 2c2 < <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> --- > <project xmlns="http://maven.apache.org/POM/4.0.0" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

So the change produces no effect on the scm info in the pom.

Have you run the build in a non git checkout?

I have not, but is that relevant for the poms we submit to maven or we just want to make sure it doesn't break ?

@rjernst This does work with non git and just produces:

<scm> <url/> </scm>

This is consistent with master.

rjernst · 2018-08-07T16:04:30Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

+            Node pom = new XmlParser()
+                    .parse(project.tasks.generatePomFileForNebulaPublication.outputs.files.singleFile)
+            pom.artifactId[0].value = project.name + "-client"
+            jarFile.resolveSibling(clientPomFileName).toFile().text = XmlUtil.serialize(pom)


please use setText(string, "UTF-8") to be explicit about the encoding

rjernst · 2018-08-07T16:08:44Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

@@ -211,35 +211,18 @@ public class PluginBuildPlugin extends BuildPlugin {

    /** Adds nebula publishing task to generate a pom file for the plugin. */
    protected static void addClientJarPomGeneration(Project project) {


Is this method still used? It looks like the one use was removed above.

rjernst · 2018-08-07T16:12:05Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

-                    jarFile.resolveSibling(clientPomFileName),
-                    StandardCopyOption.REPLACE_EXISTING
-            )
+            Node pom = new XmlParser()


Can you explain (and add a comment) why this is better than having a publishing block for the client pom? It's unclear to me what advantages this gains us.

alpar-t · 2018-08-15T08:02:59Z

@rjernst I updated this as we discussed, so that publishing and jar names are configured based on hasClient so we will never have 2 sets of artifacts. It then wanted to make the two consistent and remove applying the publishing plugin fro hasClient as well. To make sure nothing breaks I wanted to see which -client jars we publish to make sure these apply it but there aren't any in release manager, that left me puzzled on where these get published from ? I did go in and made sure that all projects that have hasClient also apply the publishing plug-in.

alpar-t · 2018-08-15T15:01:17Z

@elasticmachine test this please

rjernst

Looks better. One last thing. If you agree, no further need for review.

rjernst · 2018-08-15T21:35:32Z

modules/aggs-matrix-stats/build.gradle

@@ -16,6 +16,7 @@
 * specific language governing permissions and limitations
 * under the License.
 */
+apply plugin: 'nebula.maven-scm'


I think this is confusing to have to apply this plugin specifically. Setting hasClientJar should do this for this build.

I don't have a strong preference. The reason I changed it to this is so that's consistent with how we configure publishing without hasClientJar, where we only configure if the publishing plugin is applied.

…shing-take-2

Only configure publishing if it's applied externally, reconfigure for hasClientJar

* master: Generalize remote license checker (#32971) Trim translog when safe commit advanced (#32967) Fix an inaccuracy in the dynamic templates documentation. (#32890) Logging: Use settings when building daemon threads (#32751) All Translog inner closes should happen after tragedy exception is set (#32674) HLREST: AwaitsFix ML Test Pass DiscoveryNode to initiateChannel (#32958) Add mzn and dz to unsupported locales (#32957) Use settings from the context in BootstrapChecks (#32908) Update docs for node specifications (#30468) HLRC: Forbid all Elasticsearch logging infra (#32784) Only configure publishing if it's applied externally (#32351) Fixes libs:dissect when in eclipse Protect ScriptedMetricIT test cases against failures on 0-doc shards (#32959) (#32968) [Kerberos] Add documentation for Kerberos realm (#32662) Watcher: Properly find next valid date in cron expressions (#32734) Fix some small issues in the getting started docs (#30346) Set forbidden APIs target compatibility to compiler java version (#32935) Move connection listener to ConnectionManager (#32956)

…e-types * elastic/master: (89 commits) Fix assertion in AbstractSimpleTransportTestCase (elastic#32991) [DOC] Splits role mapping APIs into separate pages (elastic#32797) HLRC: ML Close Job (elastic#32943) Generalize remote license checker (elastic#32971) Trim translog when safe commit advanced (elastic#32967) Fix an inaccuracy in the dynamic templates documentation. (elastic#32890) Logging: Use settings when building daemon threads (elastic#32751) All Translog inner closes should happen after tragedy exception is set (elastic#32674) HLREST: AwaitsFix ML Test Pass DiscoveryNode to initiateChannel (elastic#32958) Add mzn and dz to unsupported locales (elastic#32957) Use settings from the context in BootstrapChecks (elastic#32908) Update docs for node specifications (elastic#30468) HLRC: Forbid all Elasticsearch logging infra (elastic#32784) Only configure publishing if it's applied externally (elastic#32351) Fixes libs:dissect when in eclipse Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968) [Kerberos] Add documentation for Kerberos realm (elastic#32662) Watcher: Properly find next valid date in cron expressions (elastic#32734) Fix some small issues in the getting started docs (elastic#30346) ...

Only configure publishing if it's applied externally, reconfigure for hasClientJar

* commit '9088d811ce9cff922e6ef1befbeb0f1e0c27016a': (23 commits) Generalize remote license checker (elastic#32971) Trim translog when safe commit advanced (elastic#32967) Fix an inaccuracy in the dynamic templates documentation. (elastic#32890) HLREST: AwaitsFix ML Test Make Geo Context Mapping Parsing More Strict (elastic#32862) Add mzn and dz to unsupported locales (elastic#32957) Use settings from the context in BootstrapChecks (elastic#32908) Update docs for node specifications (elastic#30468) HLRC: Forbid all Elasticsearch logging infra (elastic#32784) Fix use of deprecated apis Only configure publishing if it's applied externally (elastic#32351) Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968) Scripted metric aggregations: add deprecation warning and system (elastic#32944) Watcher: Properly find next valid date in cron expressions (elastic#32734) Fix some small issues in the getting started docs (elastic#30346) Set forbidden APIs target compatibility to compiler java version (elastic#32935) [TEST] Add "ne" as an unsupported SimpleKdc locale (elastic#32700) MINOR: Remove `IndexTemplateFilter` (elastic#32841) (elastic#32974) INGEST: Create Index Before Pipeline Execute (elastic#32786) (elastic#32975) NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (elastic#32764) (elastic#32976) ...

alpar-t requested review from nik9000 and rjernst July 25, 2018 05:52

alpar-t mentioned this pull request Jul 25, 2018

Fix multi jar publishing #32195

Closed

colings86 added the :Delivery/Build Build or test infrastructure label Jul 26, 2018

rjernst reviewed Jul 31, 2018

View reviewed changes

rjernst reviewed Aug 7, 2018

View reviewed changes

alpar-t added the WIP label Aug 10, 2018

rjernst approved these changes Aug 15, 2018

View reviewed changes

alpar-t added >non-issue v7.0.0 v6.5.0 and removed WIP labels Aug 16, 2018

alpar-t added 2 commits August 16, 2018 17:39

Only configure publishing if it's applied externally

4d061a2

Continue applying publishing

1f25694

alpar-t force-pushed the fix-multi-jar-publishing-take-2 branch from 0ecee7b to 1f25694 Compare August 16, 2018 15:05

alpar-t added 2 commits August 17, 2018 09:19

Merge remote-tracking branch 'origin/master' into fix-multi-jar-publi…

49d9bf6

…shing-take-2

Merge remote-tracking branch 'origin/master' into fix-multi-jar-publi…

01af981

…shing-take-2

alpar-t merged commit e3700a9 into elastic:master Aug 20, 2018

alpar-t deleted the fix-multi-jar-publishing-take-2 branch August 20, 2018 10:47

alpar-t added a commit that referenced this pull request Aug 20, 2018

Only configure publishing if it's applied externally (#32351)

2e06805

Only configure publishing if it's applied externally, reconfigure for hasClientJar

jasontedor pushed a commit that referenced this pull request Aug 21, 2018

Only configure publishing if it's applied externally (#32351)

2301aa5

Only configure publishing if it's applied externally, reconfigure for hasClientJar

This was referenced Jan 8, 2019

incorrect artifactId in x-pack-core Maven pom #37166

Closed

Fix artifactId in plugin poms #37315

Merged

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only configure publishing if it's applied externally #32351

Only configure publishing if it's applied externally #32351

alpar-t commented Jul 25, 2018

elasticmachine commented Jul 26, 2018

rjernst left a comment

rjernst Jul 30, 2018

alpar-t Aug 6, 2018

rjernst Jul 31, 2018

alpar-t Aug 6, 2018

alpar-t Aug 6, 2018

rjernst Aug 6, 2018

alpar-t Aug 6, 2018

alpar-t Aug 7, 2018 •

edited

Loading

rjernst Aug 7, 2018

rjernst Aug 7, 2018

rjernst Aug 7, 2018

alpar-t commented Aug 15, 2018

alpar-t commented Aug 15, 2018

rjernst left a comment

rjernst Aug 15, 2018

alpar-t Aug 16, 2018

		@@ -211,35 +211,18 @@ public class PluginBuildPlugin extends BuildPlugin {

		/** Adds nebula publishing task to generate a pom file for the plugin. */
		protected static void addClientJarPomGeneration(Project project) {

Only configure publishing if it's applied externally #32351

Only configure publishing if it's applied externally #32351

Conversation

alpar-t commented Jul 25, 2018

elasticmachine commented Jul 26, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Aug 15, 2018

alpar-t commented Aug 15, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Aug 7, 2018 •

edited

Loading