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

Add Shadow jar publication to lang-painless module. #2681

Merged
merged 4 commits into from
Apr 1, 2022

Conversation

mch2
Copy link
Member

@mch2 mch2 commented Mar 31, 2022

Signed-off-by: Marc Handalian [email protected]

Description

This change creates a shadow jar for asm dependencies so
that they do not conflict with direct asm dependencies from log4j AL2 patch.

This change also adds explicit task dependency order for generated tasks. This is breaking with latest gradle version that warns of implicit dependencies & -Werr enabled by default.

This also bumps all asm dependencies to version 9.2.

Issues Resolved

#2664

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.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 27ff98590c5f1ff3ee87b4bcbe874714883c0899
Log 3960

Reports 3960

This change creates a shadow jar for asm dependencies so
that they do not conflict with direct asm dependencies from log4j AL2 patch.

Signed-off-by: Marc Handalian <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9226ca3
Log 3965

Reports 3965

@mch2
Copy link
Member Author

mch2 commented Mar 31, 2022

❌   Gradle Check failure 9226ca3 Log 3965

Reports 3965

Tests with failures:

  • org.opensearch.cluster.remote.test.RemoteClustersIT.testHAProxyModeConnectionWorks

@mch2
Copy link
Member Author

mch2 commented Mar 31, 2022

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9226ca3
Log 3970

Reports 3970

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain in more detail what's going on here or in the original issue. This is pretty hard to grok.

Comment on lines 53 to 52
api 'org.ow2.asm:asm:9.2'
implementation 'org.ow2.asm:asm-util:9.2'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance someone was relying on this dependency and using through lang-painless module?

#### implementation

Dependencies that are used by the project at compile and runtime but are not exposed as a compile dependency to other dependent projects. Dependencies added to the `implementation` configuration are considered an implementation detail that can be changed at a later date without affecting any dependent projects.

#### api

Dependencies that are used as compile and runtime dependencies of a project and are considered part of the external api of the project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its possible, will revisit this if performing our own shadowing is the right path fwd, I'm not convinced thats the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed these back to api, but I don't think this will make a difference because we are rewriting the package namespace.

@dbbaughe
Copy link

I see these dependencies in the lang-expression module too (and a test logger), do we need to change there too?

@mch2
Copy link
Member Author

mch2 commented Mar 31, 2022

Could you please explain in more detail what's going on here or in the original issue. This is pretty hard to grok.

Added a comment on the original issue. TLDR this rewrites the namespace of these dependencies so we are not attempting to load the system dependencies injected by the hotpatch that are causing the issue.

I see these dependencies in the lang-expression module too (and a test logger), do we need to change there too?

This is stumping me, I'm not able to repro the failure running check on lang-expression with the hotpatch enabled.

@mch2 mch2 marked this pull request as ready for review April 1, 2022 19:28
@mch2 mch2 requested a review from a team as a code owner April 1, 2022 19:28
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 842d07d
Log 4037

Reports 4037

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success b38919e
Log 4038

Reports 4038

@dblock dblock merged commit d7d4108 into opensearch-project:main Apr 1, 2022
@dblock dblock added backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch labels Apr 1, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 1, 2022
* Add Shadow jar publication to lang-painless module.

This change creates a shadow jar for asm dependencies so
that they do not conflict with direct asm dependencies from log4j AL2 patch.

Signed-off-by: Marc Handalian <[email protected]>

* Remove security.manager systemProperty that is not required.

Signed-off-by: Marc Handalian <[email protected]>

* Add explicit task dependency for publishing to maven local required by gradle.

Signed-off-by: Marc Handalian <[email protected]>

* Move asm dependencies back to api scope.

Signed-off-by: Marc Handalian <[email protected]>
(cherry picked from commit d7d4108)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 1, 2022
* Add Shadow jar publication to lang-painless module.

This change creates a shadow jar for asm dependencies so
that they do not conflict with direct asm dependencies from log4j AL2 patch.

Signed-off-by: Marc Handalian <[email protected]>

* Remove security.manager systemProperty that is not required.

Signed-off-by: Marc Handalian <[email protected]>

* Add explicit task dependency for publishing to maven local required by gradle.

Signed-off-by: Marc Handalian <[email protected]>

* Move asm dependencies back to api scope.

Signed-off-by: Marc Handalian <[email protected]>
(cherry picked from commit d7d4108)
@mch2 mch2 deleted the 2664-shadow branch April 1, 2022 20:57
mch2 added a commit that referenced this pull request Apr 1, 2022
* Add Shadow jar publication to lang-painless module.

This change creates a shadow jar for asm dependencies so
that they do not conflict with direct asm dependencies from log4j AL2 patch.

Signed-off-by: Marc Handalian <[email protected]>

* Remove security.manager systemProperty that is not required.

Signed-off-by: Marc Handalian <[email protected]>

* Add explicit task dependency for publishing to maven local required by gradle.

Signed-off-by: Marc Handalian <[email protected]>

* Move asm dependencies back to api scope.

Signed-off-by: Marc Handalian <[email protected]>
(cherry picked from commit d7d4108)

Co-authored-by: Marc Handalian <[email protected]>
mch2 added a commit that referenced this pull request Apr 1, 2022
* Add Shadow jar publication to lang-painless module.

This change creates a shadow jar for asm dependencies so
that they do not conflict with direct asm dependencies from log4j AL2 patch.

Signed-off-by: Marc Handalian <[email protected]>

* Remove security.manager systemProperty that is not required.

Signed-off-by: Marc Handalian <[email protected]>

* Add explicit task dependency for publishing to maven local required by gradle.

Signed-off-by: Marc Handalian <[email protected]>

* Move asm dependencies back to api scope.

Signed-off-by: Marc Handalian <[email protected]>
(cherry picked from commit d7d4108)

Co-authored-by: Marc Handalian <[email protected]>
@dblock
Copy link
Member

dblock commented Apr 4, 2022

@mch2 Will we want to revert this at some point when the hot patching issue is generally fixed? open an issue?

@mch2
Copy link
Member Author

mch2 commented Apr 4, 2022

@mch2 Will we want to revert this at some point when the hot patching issue is generally fixed? open an issue?

Yes imo we should revert - added issue #2753

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants