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

Fix error InvalidMavenPublicationException #554

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

prudhvigodithi
Copy link
Member

@prudhvigodithi prudhvigodithi commented Oct 13, 2022

Description

Fix error org.gradle.api.publish.maven.InvalidMavenPublicationException:
Coming from the comment #549 (comment)

Issues Resolved

#501

Check List

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

@prudhvigodithi prudhvigodithi requested a review from a team October 13, 2022 23:54
@prudhvigodithi prudhvigodithi added backport 2.x v2.4.0 'Issues and PRs related to version v2.4.0' labels Oct 13, 2022
@prudhvigodithi prudhvigodithi changed the title Fix error org.gradle.api.publish.maven.InvalidMavenPublicationException: Fix error org.gradle.api.publish.maven.InvalidMavenPublicationException Oct 13, 2022
@prudhvigodithi prudhvigodithi changed the title Fix error org.gradle.api.publish.maven.InvalidMavenPublicationException Fix error InvalidMavenPublicationException Oct 13, 2022
@prudhvigodithi
Copy link
Member Author

Build pass on my local with this change
./build.sh manifests/2.4.0/opensearch-2.4.0.yml --component notifications-core, ./build.sh manifests/2.4.0/opensearch-2.4.0.yml --component notifications

maven folder with org.opensearch.plugin

pwd
/usr/share/opensearch/git/opensearch-build/tar/builds/opensearch/maven/org/opensearch/plugin
[opensearch@ee8276647b38 plugin]$ ls -ltr
total 8
drwxrwxr-x 3 opensearch opensearch 4096 Oct 13 23:49 notifications
drwxrwxr-x 3 opensearch opensearch 4096 Oct 13 23:50 opensearch-notifications-core

@lezzago lezzago merged commit 35a091a into opensearch-project:main Oct 14, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 14, 2022
Signed-off-by: prudhvigodithi <[email protected]>

Signed-off-by: prudhvigodithi <[email protected]>
(cherry picked from commit 35a091a)
lezzago pushed a commit that referenced this pull request Oct 14, 2022
Signed-off-by: prudhvigodithi <[email protected]>

Signed-off-by: prudhvigodithi <[email protected]>
(cherry picked from commit 35a091a)

Co-authored-by: Prudhvi Godithi <[email protected]>
@lukas-vlcek
Copy link

lukas-vlcek commented Oct 14, 2022

@prudhvigodithi This is interesting.

Right now I am not sure why this is happening. But I noticed one interesting thing, without your modification from this PR I am still able to build both plugins individually like this:

# Let's get to the root notifications folder which hosts subprojects (the plugins)
$ cd notifications

# Start with the core plugin
$ cd core
$ ../gradlew clean publishToMavenLocal

# Switch to notification plugin and build it
$ cd ../notifications
$ ../gradlew clean publishToMavenLocal

And now I can see the following in my local mvn repo

$ pwd
/Users/lukas.vlcek/.m2/repository/org/opensearch/plugin

$ ls -ltr
drwxr-xr-x  4 lukas.vlcek  staff  128 Oct 14 20:57 opensearch-notifications-core
drwxr-xr-x  4 lukas.vlcek  staff  128 Oct 14 20:59 notifications

So both the plugins were build correctly (at least it seems to me).
Except I am not just sure why the notifications plugin is deployed into notifications folder and not into opensearch-notifications folder, but this might be different issue.

It feels to me that something is stepping on each other's toes when gradle is run from the parent folder (which I think is the case of the opensearch-build script).

@lukas-vlcek
Copy link

lukas-vlcek commented Oct 17, 2022

@prudhvigodithi

I was looking at this again and I think for the time being we could consider the following workaround:

Instead of this, which fails with exception:

./gradlew publishToMavenLocal

We can run the tasks for all subprojects individually. Not great but we could live with it until we find a better fix?:

./gradlew :notifications:publishToMavenLocal
./gradlew :opensearch-notifications-core:publishToMavenLocal   ## Getting warning but it does not fail
./gradlew :opensearch-notifications-core-spi:publishToMavenLocal

BTW, I think there is a related issue in gradle gradle/gradle#6009 which was closed by gradle/gradle#9465
The error message in the issue has a slightly different wording but it was changed exactly to what we observe from our build, which makes me think that maybe we are able to hit this issues now. But this is just a speculation on my side. I will continue digging deeper.

@prudhvigodithi
Copy link
Member Author

Hey @lukas-vlcek thanks, if so we should consider modifying the notifications-core and notifications build scripts and execute the build workflow
Example:
./build.sh manifests/2.4.0/opensearch-2.4.0.yml --component notifications-core
./build.sh manifests/2.4.0/opensearch-2.4.0.yml --component notifications

Once this build successfully we can then remove the added setting startParameter.excludedTaskNames=["publishPluginZipPublicationToMavenLocal"] wdyt?
Thank you

@lukas-vlcek
Copy link

The is also a nebule plugin upgrade PR going on opensearch-project/OpenSearch#4810 (it might be relevant, or not... I do not know atm). Let's keep a reference this PR here (even if it is for main).

@lukas-vlcek
Copy link

lukas-vlcek commented Oct 18, 2022

I had a short chat with @prudhvigodithi about this yesterday and the following is the summary:

TL;DR: We are ok for now to move forward with this PR as it is.

More details:

For now we are ok with the workaround provided in this PR as it is unblocking the build process. The side-effect of this PR is that ZIP publication artifacts for OpenSearch plugins (opensearch.opensearchplugin) are not deployed into local maven repository for the publishToMavenLocal task, but in fact they are not needed for the local repo anyway (unlike to the staging repo), so this is acceptable compromise.

It would make sense to try to run the build for each individual plugin as suggested and demonstrated above because this does not lead to the exception and all plugins are built as expected. But that would require changes to the build scripts itself and that can be a risk that we do not want to take now given we are approaching 2.4 code freeze soon.


This being said there are potentially two issues that we need to keep an eye on in the future:

  1. The exception that was thrown for ./gradlew publishToMavenLocal command before this PR has been applied. At this point the source of this problem is not identified. Maybe all it takes is to upgrade the nebula library (see #4810).
  2. The gradle warning for ./gradlew :opensearch-notifications-core:publishToMavenLocal command:
Task :opensearch-notifications-core:generatePomFileForPluginZipPublication
Execution optimizations have been disabled for task ':opensearch-notifications-core:generatePomFileForPluginZipPublication' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/Users/lukas.vlcek/projects/opensearch-project/notifications/notifications/core/build/distributions/opensearch-notifications-core-2.4.0.0-SNAPSHOT.pom'. Reason: Task ':opensearch-notifications-core:publishNebulaPublicationToMavenLocal' uses this output of task ':opensearch-notifications-core:generatePomFileForPluginZipPublication' 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants