Gradle plugin `opensearch.pluginzip` Add implicit dependency. #3189

prudhvigodithi · 2022-05-04T21:11:01Z

Signed-off-by: pgodithi [email protected]

Description

This fix will remove the following warning's during runtime

Execution optimizations have been disabled for task ':publishPluginZipPublicationToZipStagingRepository' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: 'opensearch-plugin-template-java/build/distributions/rename-unspecified.pom'. Reason: Task ':publishPluginZipPublicationToZipStagingRepository' uses this output of task ':generatePomFileForNebulaPublication' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

> Task :validatePluginZipPom FAILED
Execution optimizations have been disabled for task ':validatePluginZipPom' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom'. Reason: Task ':validatePluginZipPom' uses this output of task ':generatePomFileForNebulaPublication' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.
name is missing in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
description is missing in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
licenses is empty in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
developers is empty in [/usr/share/opensearch/git/opensearch-plugin-template-java/build/distributions/rename-unspecified.pom]
Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

Issues Resolved

#3183
opensearch-project/opensearch-plugin-template-java#31

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: pgodithi <[email protected]>

opensearch-ci-bot · 2022-05-04T21:27:38Z

❌ Gradle Check failure b9f2e3a
Log 5009

Reports 5009

Signed-off-by: pgodithi <[email protected]>

opensearch-ci-bot · 2022-05-04T21:27:49Z

❌ Gradle Check failure 1b5f1ba
Log 5010

Reports 5010

opensearch-ci-bot · 2022-05-04T21:29:57Z

❌ Gradle Check failure 2402d78
Log 5011

Reports 5011

prudhvigodithi · 2022-05-04T21:53:59Z

Hey @reta can you please validate this, this should remove those generate warning's.
@dblock

opensearch-ci-bot · 2022-05-04T21:57:59Z

✅ Gradle Check success b7c93f0
Log 5013

Reports 5013

reta · 2022-05-05T12:37:47Z

buildSrc/src/main/java/org/opensearch/gradle/pluginzip/Publish.java

+            configMaven(project);
+            Task validatePluginZipPom = project.getTasks().findByName("validatePluginZipPom");
+            if (validatePluginZipPom != null) {
+                project.getTasks().getByName("validatePluginZipPom").mustRunAfter("generatePomFileForNebulaPublication");


Just curious, why mustRunAfter but not dependsOn? We do not rely on any order, right?

mustRunAfter controls the execution order of tasks explicitly, so this ensures the task generatePomFileForNebulaPublication is called before the validatePluginZipPom and publishPluginZipPublicationToZipStagingRepository

dependsOn does the same, mustRunAfter mandates that task has to run exactly in order after the task in question: I don't think we have such exact order requirements, dependsOn should be sufficient

mustRunAfter would ensure that task has been called out before those tasks. I have tested with mustRunAfter and I dont see the warnings anymore, as mustRunAfter calls that task before the dependent tasks @reta

But both works here 🙂
I'm fine with mustRunAfter or dependsOn, open for the change thoughts @reta @dblock ?:)

I would suggest to switch to dependsOn, it introduces implicit dependencies (which is what we need, because our publication depends on it) [1]:

A task may have dependencies on other tasks or might be scheduled to always run after another task. Gradle ensures that all task dependencies and ordering rules are honored when executing tasks, so that the task is executed after all of its dependencies and any "must run after" tasks have been executed.

but mustRunAfter does merely controls the order [2]:

When you use the “must run after” ordering rule you specify that taskB must always run after taskA, whenever both taskA and taskB will be run.

The difference is actually very easy to demonstrate: if you just run a single publishPluginZipPublicationToZipStagingRepository task, none of the mustRunAfter tasks will be executed (notice generatePomFileForNebulaPublication is not there):

./gradlew -m publishPluginZipPublicationToZipStagingRepository :generateNotice SKIPPED :compileJava SKIPPED :processResources SKIPPED :classes SKIPPED :jar SKIPPED :copyPluginPropertiesTemplate SKIPPED :pluginProperties SKIPPED :bundlePlugin SKIPPED :generatePomFileForPluginZipPublication SKIPPED :publishPluginZipPublicationToZipStagingRepository SKIPPED

Now, using dependsOn (notice generatePomFileForNebulaPublication is there):

./gradlew -m publishPluginZipPublicationToZipStagingRepository :generateNotice SKIPPED :compileJava SKIPPED :processResources SKIPPED :classes SKIPPED :jar SKIPPED :copyPluginPropertiesTemplate SKIPPED :pluginProperties SKIPPED :bundlePlugin SKIPPED :generatePomFileForNebulaPublication SKIPPED :generatePomFileForPluginZipPublication SKIPPED :publishPluginZipPublicationToZipStagingRepository SKIPPED

Hope it is sufficient to justify dependsOn usage.

[1] https://docs.gradle.org/current/dsl/org.gradle.api.Task.html#N18A16
[2] https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:ordering_tasks

But both works here slightly_smiling_face

I hope the examples above help to clarify when it does not work

Thanks for the info @reta .
-m shows the task actions disabled, here our initial error is because we have a task generatePomFileForNebulaPublication that has been not called and these validatePluginZipPom & publishPluginZipPublicationToZipStagingRepository actually uses the output of generatePomFileForNebulaPublication, using mustRun will ensure this task generatePomFileForNebulaPublication has been invoked when ever the tasks validatePluginZipPom & publishPluginZipPublicationToZipStagingRepository are triggered, which should solve the output dependency, hence I mentioned running build should work, even though it does not show with -m output mustRun ensures it will execute the task.

But that said dependsOn should also work as we are adding exclusive graph dependency, I have updated my latest commit with dependsOn and ran few plugins build.sh and runs fine, please check @reta

reta · 2022-05-05T12:43:43Z

buildSrc/src/main/java/org/opensearch/gradle/pluginzip/Publish.java

+            if (validatePluginZipPom != null) {
+                project.getTasks().getByName("validatePluginZipPom").mustRunAfter("generatePomFileForNebulaPublication");
+            }
+            Task publishPluginZipPublicationToZipStagingRepository = project.getTasks()


I think we should not use exact task name here but rely on the task type as per [1]:

project.getTasks().withType(PublishToMavenRepository.class).configureEach(t -> { if (t.getPublication().getName().equals(PUBLICATION_NAME)) { t.dependsOn(pomFileTask); } });

The reason for it is that we may publish to different repositories (fe local Maven) and that would be different task name, still requiring same task dependencies.

[1] https://github.com/opensearch-project/OpenSearch/pull/3183/files

True, I already have PUBLICATION_NAME var declared I can use that. I will push the change shortly.

Hey @reta I assumed wrong, my bad, here we should be good, using this Plugin we dont push to any other repos, its strictly to pluginZip repo that generates fixed task publishPluginZipPublicationToZipStagingRepository, hence we should be good to hardcode, initially I have created few utils that can identify the PUBLICATION_NAME and taskname, but since its only single publication here using this plugin, I have had some conversations in this merged PR to hard code things, so long story short we should be good to hardcode task names.

Signed-off-by: pgodithi <[email protected]>

prudhvigodithi · 2022-05-05T18:26:50Z

Thanks @reta

opensearch-ci-bot · 2022-05-05T18:44:57Z

✅ Gradle Check success fa56ddd
Log 5044

Reports 5044

dblock · 2022-05-05T21:10:52Z

@reta You should feel free to merge anything that you see fit without any additional approvals.

* Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add dependsOn Signed-off-by: pgodithi <[email protected]> (cherry picked from commit 2fe2e37)

reta · 2022-05-05T21:53:33Z

@reta You should feel free to merge anything that you see fit without any additional approvals.

Thanks @dblock! I thought we are merging with 2 approvals (no restrictions on that, just general observation), no?

dblock · 2022-05-06T21:49:33Z

@reta You should feel free to merge anything that you see fit without any additional approvals.

Thanks @dblock! I thought we are merging with 2 approvals (no restrictions on that, just general observation), no?

Nah, I think we relaxed that a while ago.

…#3211) * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add dependsOn Signed-off-by: pgodithi <[email protected]> (cherry picked from commit 2fe2e37) Co-authored-by: Prudhvi Godithi <[email protected]>

…#3212) * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add implicit dependency Signed-off-by: pgodithi <[email protected]> * Add dependsOn Signed-off-by: pgodithi <[email protected]> (cherry picked from commit 2fe2e37) Co-authored-by: Prudhvi Godithi <[email protected]>

Add implicit dependency

b9f2e3a

Signed-off-by: pgodithi <[email protected]>

prudhvigodithi requested review from a team and reta as code owners May 4, 2022 21:11

prudhvigodithi added 4 commits May 4, 2022 17:13

Add implicit dependency

ebb4ecb

Signed-off-by: pgodithi <[email protected]>

Add implicit dependency

1b5f1ba

Signed-off-by: pgodithi <[email protected]>

Add implicit dependency

451676f

Signed-off-by: pgodithi <[email protected]>

Add implicit dependency

2402d78

Signed-off-by: pgodithi <[email protected]>

Add implicit dependency

b7c93f0

Signed-off-by: pgodithi <[email protected]>

reta reviewed May 5, 2022

View reviewed changes

Add dependsOn

fa56ddd

Signed-off-by: pgodithi <[email protected]>

reta approved these changes May 5, 2022

View reviewed changes

prudhvigodithi mentioned this pull request May 5, 2022

Fixing publishZip publication by adding the dependency to generatePomFileForNebulaPublication #3183

Closed

5 tasks

dblock approved these changes May 5, 2022

View reviewed changes

dblock merged commit 2fe2e37 into opensearch-project:main May 5, 2022

dblock added the backport 2.x Backport to 2.x branch label May 5, 2022

opensearch-trigger-bot bot mentioned this pull request May 5, 2022

[Backport 2.x] Gradle plugin opensearch.pluginzip Add implicit dependency. #3211

Merged

dblock added the backport 2.0 Backport to 2.0 branch label May 5, 2022

opensearch-trigger-bot bot mentioned this pull request May 5, 2022

[Backport 2.0] Gradle plugin opensearch.pluginzip Add implicit dependency. #3212

Merged

prudhvigodithi mentioned this pull request May 6, 2022

Added opensearch.pluginzip plugin opensearch-project/opensearch-plugin-template-java#31

Merged

prudhvigodithi mentioned this pull request Aug 31, 2022

Update OpenSearch version to 3.0.0-SNAPSHOT opensearch-project/opensearch-plugin-template-java#35

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gradle plugin `opensearch.pluginzip` Add implicit dependency. #3189

Gradle plugin `opensearch.pluginzip` Add implicit dependency. #3189

prudhvigodithi commented May 4, 2022 •

edited

Loading

opensearch-ci-bot commented May 4, 2022

opensearch-ci-bot commented May 4, 2022

opensearch-ci-bot commented May 4, 2022

prudhvigodithi commented May 4, 2022

opensearch-ci-bot commented May 4, 2022

reta May 5, 2022 •

edited

Loading

prudhvigodithi May 5, 2022

reta May 5, 2022

prudhvigodithi May 5, 2022 •

edited

Loading

prudhvigodithi May 5, 2022 •

edited

Loading

reta May 5, 2022 •

edited

Loading

reta May 5, 2022

prudhvigodithi May 5, 2022

reta May 5, 2022

prudhvigodithi May 5, 2022

prudhvigodithi May 5, 2022

prudhvigodithi commented May 5, 2022

opensearch-ci-bot commented May 5, 2022

dblock commented May 5, 2022

reta commented May 5, 2022

dblock commented May 6, 2022

Gradle plugin opensearch.pluginzip Add implicit dependency. #3189

Gradle plugin opensearch.pluginzip Add implicit dependency. #3189

Conversation

prudhvigodithi commented May 4, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented May 4, 2022

opensearch-ci-bot commented May 4, 2022

opensearch-ci-bot commented May 4, 2022

prudhvigodithi commented May 4, 2022

opensearch-ci-bot commented May 4, 2022

reta May 5, 2022 • edited Loading

Choose a reason for hiding this comment

prudhvigodithi May 5, 2022

Choose a reason for hiding this comment

reta May 5, 2022

Choose a reason for hiding this comment

prudhvigodithi May 5, 2022 • edited Loading

Choose a reason for hiding this comment

prudhvigodithi May 5, 2022 • edited Loading

Choose a reason for hiding this comment

reta May 5, 2022 • edited Loading

Choose a reason for hiding this comment

reta May 5, 2022

Choose a reason for hiding this comment

prudhvigodithi May 5, 2022

Choose a reason for hiding this comment

reta May 5, 2022

Choose a reason for hiding this comment

prudhvigodithi May 5, 2022

Choose a reason for hiding this comment

prudhvigodithi May 5, 2022

Choose a reason for hiding this comment

prudhvigodithi commented May 5, 2022

opensearch-ci-bot commented May 5, 2022

dblock commented May 5, 2022

reta commented May 5, 2022

dblock commented May 6, 2022

Gradle plugin `opensearch.pluginzip` Add implicit dependency. #3189

Gradle plugin `opensearch.pluginzip` Add implicit dependency. #3189

prudhvigodithi commented May 4, 2022 •

edited

Loading

reta May 5, 2022 •

edited

Loading

prudhvigodithi May 5, 2022 •

edited

Loading

prudhvigodithi May 5, 2022 •

edited

Loading

reta May 5, 2022 •

edited

Loading