You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/02/09 15:30:40 UTC
[GitHub] [geode] ryangardner opened a new pull request #7351: Upgrade to Gradle 7.4
ryangardner opened a new pull request #7351:
URL: https://github.com/apache/geode/pull/7351
Upgrade to Gradle 7.4
- Updated gradle wrapper
- Updated usage of an internal TemporaryFileProvider to the correct location for Gradle 7
- Update the `main` -> `mainClass` for the geode assembly to match Gradle 7 syntax
- Move some `compile` scope to `compileOnly` (required for Gradle 7)
- Add explicit `dependsOn` relationship between tasks that have dependencies that Gradle 7 complains about
- Add a `duplicatesStrategy` to the processIntegrationTestResources to avoid Gradle 7 failing on a duplicate resources in shiro.ini (this duplicate _seems_ to come from how the source set is configured)
- Upgrade the RepeatTestExecutor to match the DefaultTestExecutor from Gradle 7.4
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- I couldn't find one
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- I have some errors with bind exceptions for some of the tests, but those also happen on the gradle 6.8 build for me and I haven't looked deeply into them.
- [ ] Have you written or updated unit tests to verify your changes?
N/A
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ryangardner commented on pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
ryangardner commented on pull request #7351:
URL: https://github.com/apache/geode/pull/7351#issuecomment-1051385302
Sorry, I've been away from my computer this week - I saw your earlier comment about updating the commit message and I can still do that, I've just not had a chance yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r807119954
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
@ryangardner I tested your PR on my side, and we can remove the line 423 addition above. It isn't part of the warning, and `geode-old-versions:test` has no real actions so it resolves to a no-op statement anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r807119954
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
@ryangardner I tested your PR on my side, and we can remove the line 423 addition above. It isn't part of the warning, and `geode-old-versions:test` has no real actions so it resolves to a no-op statement anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ryangardner commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
ryangardner commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r803774213
##########
File path: geode-assembly/src/acceptanceTest/resources/gradle-test-projects/management/build.gradle
##########
@@ -24,7 +24,7 @@ repositories {
}
dependencies {
- compile("${project.group}:geode-core:${project.version}")
+ compileOnly("${project.group}:geode-core:${project.version}")
Review comment:
Good catch, it worked locally on my mac as well - I just pushed up something with that change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #7351:
URL: https://github.com/apache/geode/pull/7351#issuecomment-1051387835
Hi Ryan, I don't know if you're subscribed dev@geode.apache.org, I just posted an [announcement](https://lists.apache.org/thread/y3s11x39lwsmxjyw7y7oznlqolszyjn6) there clarifying things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ryangardner commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
ryangardner commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r804285064
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
Sure, no rush... I wasn't trying to create work / re-prioritize work for anyone by making this PR, it was more intended as a "in case this is helpful... this might be a starting point on getting to Gradle 7..."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ryangardner commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
ryangardner commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r804079070
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
Without that dependency declared, it fails with the following error message:
```
> Task :geode-core:test FAILED
> Task :combineReports
Execution optimizations have been disabled for task ':combineReports' to ensure correctness due to the following reasons:
- Gradle detected a problem with the following location: 'geode/geode-old-versions/build/test-results/test/binary'. Reason: Task ':combineReports' uses this output of task ':geode-old-versions:test' 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/userguide/validation_problems.html#implicit_dependency for more details about this problem.
```
The combine reports task seems to depend upon things coming from the old-versions - there are some IP bind failures (three actual test failures) in the geode-core tests for me as well so it's not clear if this message would just be considered a warning or not.
There might be a way to modify the `:combineReports` task to make it not use the output of the `geode-old-versions:test` if it doesn't need to run that test and that would also get rid of that warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r804247319
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
I'll try and work out how to mitigate that task. Give me a day :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r804053495
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
I think this line will cause the `geode-old-versions` to be downloaded any time we run tests. We want to only get those archives for the upgrade tests, to make the feedback loop for developers faster.
What error or warning was the cause of this addition?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r803205195
##########
File path: geode-assembly/src/acceptanceTest/resources/gradle-test-projects/management/build.gradle
##########
@@ -24,7 +24,7 @@ repositories {
}
dependencies {
- compile("${project.group}:geode-core:${project.version}")
+ compileOnly("${project.group}:geode-core:${project.version}")
Review comment:
On my Mac, if I change this to `implementation`, the `GradleBuildWithGeodeCoreAcceptanceTest` completes without error on both JDK 8 and JDK 11. I suspect this change will also fix on Windows assembly test failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ryangardner commented on a change in pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
ryangardner commented on a change in pull request #7351:
URL: https://github.com/apache/geode/pull/7351#discussion_r804079070
##########
File path: geode-core/build.gradle
##########
@@ -420,4 +420,10 @@ distributedTest {
exclude "**/*\$*.class"
}
+rootProject.combineReports.dependsOn(':geode-old-versions:test')
Review comment:
Without that dependency declared, it fails with the following error message:
```
> Task :geode-core:test FAILED
> Task :combineReports
Execution optimizations have been disabled for task ':combineReports' to ensure correctness due to the following reasons:
- Gradle detected a problem with the following location: 'geode/geode-old-versions/build/test-results/test/binary'. Reason: Task ':combineReports' uses this output of task ':geode-old-versions:test' 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/userguide/validation_problems.html#implicit_dependency for more details about this problem.
```
The combine reports task seems to depend upon things coming from the old-versions - though it _might just be a warning_ because there are some IP bind failures (three actual test failures) in the geode-core tests for me as well.
There might be a way to modify the `:combineReports` task to make it not use the output of the `geode-old-versions:test` if it doesn't need to run that test and that would also get rid of that warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] ryangardner commented on pull request #7351: Upgrade to Gradle 7.4
Posted by GitBox <gi...@apache.org>.
ryangardner commented on pull request #7351:
URL: https://github.com/apache/geode/pull/7351#issuecomment-1051391105
Ah, ok. I wasn't offended by the message but the announcement clarifies the
intent. Thanks.
Ryan
On Fri, Feb 25, 2022, 7:12 PM Owen Nichols ***@***.***> wrote:
> Hi Ryan, I don't know if you're subscribed ***@***.***, I just
> posted an announcement
> <https://lists.apache.org/thread/y3s11x39lwsmxjyw7y7oznlqolszyjn6> there
> clarifying things.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/geode/pull/7351#issuecomment-1051387835>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABZOVLHUTGETGTIXUFR5HTU5ALHLANCNFSM5N52WGGQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org