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

[Backport] [1.x] Support Gradle 7 (#1609) #1622

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

reta
Copy link
Collaborator

@reta reta commented Nov 29, 2021

Signed-off-by: Andriy Redko [email protected]

Description

Backport of #1609 to 1.x

Issues Resolved

Closes #1246

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 Wrapper Validation success 7e312662d21178a972d72bedd8e29af94dca9f29

@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 7e312662d21178a972d72bedd8e29af94dca9f29

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 7e312662d21178a972d72bedd8e29af94dca9f29
Log 1260

Reports 1260

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

Looks like this PR contains some of the changes from PR #1057.

@reta
Copy link
Collaborator Author

reta commented Nov 29, 2021

Looks like this PR contains some of the changes from PR #1057.

@VachaShah That is correct, jcenter is deprecated feature and Gradle 7.x now fails the build (although I didn't know about this pull request, thank you)

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success e9f901dede889759da4b06bfc0019c1bb7e3971b

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Precommit failure e9f901dede889759da4b06bfc0019c1bb7e3971b
Log 1661

@reta
Copy link
Collaborator Author

reta commented Nov 30, 2021

❌ Gradle Precommit failure e9f901d Log 1661

Hm ... the log is 0 bytes ...

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success e9f901dede889759da4b06bfc0019c1bb7e3971b
Log 1265

Reports 1265

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 5fded05caa9b0bed58b6998ce48d07073cc50787

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 5fded05caa9b0bed58b6998ce48d07073cc50787

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 5fded05caa9b0bed58b6998ce48d07073cc50787
Log 1269

Reports 1269

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 3ea76d641b9d4e0fa92fc8eabb5d572d6070fdcc

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 3ea76d641b9d4e0fa92fc8eabb5d572d6070fdcc

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 3ea76d641b9d4e0fa92fc8eabb5d572d6070fdcc
Log 1270

Reports 1270

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 1de370009a356f0197c9a2c77ce79ed683379331

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 1de370009a356f0197c9a2c77ce79ed683379331

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 1de370009a356f0197c9a2c77ce79ed683379331
Log 1271

Reports 1271

@reta
Copy link
Collaborator Author

reta commented Nov 30, 2021

The WhiteSource check is failing on 1.x (known issue)

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 1db2fffe33e73c02d09593203b19ceb6cd5c83af

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Precommit failure 1db2fffe33e73c02d09593203b19ceb6cd5c83af
Log 1688

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 1db2fffe33e73c02d09593203b19ceb6cd5c83af
Log 1307

Reports 1307

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 9154f86

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 9154f86

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9154f86
Log 1312

Reports 1312

Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

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

This change may introduce breaking changes for plugin authors. Should they be in the 1.x series?

It appears that some of these changes require updating to Gradle 7. I tried building with Gradle 6.6.1 and 6.8.1 against the 2.0.0-SNAPSHOT. The build fails fails due to missing classes. As plugin author, I expect that breaking changes would only come from changes to the major version.

If a plugin were to update to use OpenSearch 1.3.0, then their existing Gradle build would break.

I'm speaking more about independent plugin authors rather than the OpenSearch maintained plugins.

import org.gradle.api.provider.ProviderFactory;
import org.gradle.internal.jvm.Jvm;
import org.gradle.jvm.toolchain.JavaInstallation;
import org.gradle.jvm.toolchain.JavaInstallationRegistry;
import org.gradle.internal.jvm.inspection.JvmInstallationMetadata;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this use a package from org.gradle.internal? It seems this is not intended for usage and not documented in the Javadocs.

I do see that this made it into main and this is just the backport.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dlvenable the Opensearch build manipulates JVM installations on several occasions, Gradle 7 changes the underlying implementation significantly, so the 2 options we had: a) find the way to keep manipulating JVM installations (sadly, using internal classes) b) scrape and rewrite large chunks of the build (using toolchains or alike). The first option looked like reasonable tradeoff from efforts perspective, but I would be happy to see better solution.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @reta for the explanation. That does sound like a reasonable tradeoff. I do think that a better long-term solution would be to use the new toolchains feature. It would take some upfront work, but should also allow easier updating of Gradle versions since it will not be tied to internal implementations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 no objections!

@reta
Copy link
Collaborator Author

reta commented Dec 3, 2021

@dlvenable

This change may introduce breaking changes for plugin authors. Should they be in the 1.x series?

That is true, it will. The unfortunate part is, the Gradle is moving forward, and we just cannot keep 1.x forever on old Gradle distributions. The good news, for plugin authors it should be an easy change - update the wrapper and that is it (unless builds are heavily customized).

If a plugin were to update to use OpenSearch 1.3.0, then their existing Gradle build would break.

Correct.

I'm speaking more about independent plugin authors rather than the OpenSearch maintained plugins.

There is also the other side - because of OpenSearch outdated Gradle distribution, the plugin authors cannot use newer Gradle versions.

Thank you

@dlvenable
Copy link
Member

dlvenable commented Dec 3, 2021

@reta, Thanks for your work on this.

There is also the other side - because of OpenSearch outdated Gradle distribution, the plugin authors cannot use newer Gradle versions.

Data Prepper has been experience this, so I agree here!

From my experience with Gradle, they have done a good job of giving long deprecation paths for features. So I'm also wondering if there is a way to make the build work for both as a migration path.

In my limited build test, this is the error I had in 6.8.1:

A problem occurred evaluating project ':data-prepper-plugins:opensearch'.
> Failed to apply plugin class 'org.opensearch.gradle.info.GlobalBuildInfoPlugin'.
   > Could not create plugin of type 'GlobalBuildInfoPlugin'.
      > Could not generate a decorated class for type GlobalBuildInfoPlugin.
         > org/gradle/internal/jvm/inspection/JvmInstallationMetadata

@reta
Copy link
Collaborator Author

reta commented Dec 3, 2021

Thanks @dlvenable

From my experience with Gradle, they have done a good job of giving long deprecation paths for features. So I'm also wondering if there is a way to make the build work for both as a migration path.

Hm, good question, I could certainly look into that

@dblock dblock merged commit 93bd32b into opensearch-project:1.x Dec 3, 2021
@reta
Copy link
Collaborator Author

reta commented Dec 3, 2021

@dlvenable so I looked into what could we do to support both Gradle 6.x and 7.x, it turned out that this particular aspect of GlobalBuildInfoPlugin (JavaInstallation, ...) didn't go through deprecation cycle, it just got removed, ... In theory, throwing Class.forName and friends at a problem could solve it, it is going to be ugly.

I don't recall any mechanism in Gradle to apply condition logic / configuration based on Gradle version, if you have any ideas - please share.

@dlvenable
Copy link
Member

@reta ,

That makes sense. Since it is internal, they wouldn't take the deprecation path.

I don't recall any mechanism in Gradle to apply condition logic / configuration based on Gradle version, if you have any ideas - please share.

You can use project.getGradle().getGradleVersion() to perform conditional logic. But, this would be hard to test. You'd need to have use Gradle TestKit and GradleRunner to verify that the plugin works in both versions of Gradle. It might not really be worth it. Especially since the GlobalBuildInfoPlugin doesn't have any tests right now.

@reta
Copy link
Collaborator Author

reta commented Dec 3, 2021

@dlvenable

That makes sense. Since it is internal, they wouldn't take the deprecation path.

I would have 100% agreed but they were not internal:

import org.gradle.jvm.toolchain.JavaInstallation;
import org.gradle.jvm.toolchain.JavaInstallationRegistry;

You can use project.getGradle().getGradleVersion() to perform conditional logic. But, this would be hard to test. You'd need to have use Gradle TestKit and GradleRunner to verify that the plugin works in both versions of Gradle. It might not really be worth it. Especially since the GlobalBuildInfoPlugin doesn't have any tests right now.

Oh thank you, good to know it is there. Funny, but we have this DependencyLicensesTaskTests test case (in build tools) which checks the licenses manipulating Groovy deps from Gradle's distribution, .... and it won't work with both :D

@dlvenable
Copy link
Member

It appears that those were incubating features: https://github.com/gradle/gradle/blob/v6.6.1/subprojects/platform-jvm/src/main/java/org/gradle/jvm/toolchain/JavaInstallation.java

reta added a commit to reta/OpenSearch that referenced this pull request Dec 6, 2021
dblock pushed a commit that referenced this pull request Dec 6, 2021
This reverts commit 93bd32b.

Signed-off-by: Andriy Redko <[email protected]>
peternied added a commit to peternied/security that referenced this pull request Mar 2, 2022
BWC tooling is built with gradle 6, which has breaking changes that are
not compatiable with gradle 7.  In order to support BWC tests we need to
align with the OpenSearch's gradle version for the 1.3 release.

See Also:
* Gradle 7 PR in OpenSearch opensearch-project/OpenSearch#1622
* Distribution build bugs encountered by plugins opensearch-project/opensearch-build#1247
* Revert of Gradle 7 PR in OpenSearch  opensearch-project/OpenSearch#1657

Signed-off-by: Peter Nied <[email protected]>
peternied added a commit to opensearch-project/security that referenced this pull request Mar 15, 2022
* Downgrade gradle version

BWC tooling is built with gradle 6, which has breaking changes that are
not compatiable with gradle 7.  In order to support BWC tests we need to
align with the OpenSearch's gradle version for the 1.3 release.

See Also:
* Gradle 7 PR in OpenSearch opensearch-project/OpenSearch#1622
* Distribution build bugs encountered by plugins opensearch-project/opensearch-build#1247
* Revert of Gradle 7 PR in OpenSearch  opensearch-project/OpenSearch#1657

Signed-off-by: Peter Nied <[email protected]>
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
* Downgrade gradle version

BWC tooling is built with gradle 6, which has breaking changes that are
not compatiable with gradle 7.  In order to support BWC tests we need to
align with the OpenSearch's gradle version for the 1.3 release.

See Also:
* Gradle 7 PR in OpenSearch opensearch-project/OpenSearch#1622
* Distribution build bugs encountered by plugins opensearch-project/opensearch-build#1247
* Revert of Gradle 7 PR in OpenSearch  opensearch-project/OpenSearch#1657

Signed-off-by: Peter Nied <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants