You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by GitBox <gi...@apache.org> on 2023/01/09 08:56:45 UTC

[GitHub] [jmeter] sandra-thieme opened a new pull request, #5757: Build improvements

sandra-thieme opened a new pull request, #5757:
URL: https://github.com/apache/jmeter/pull/5757

   ## Description
   This PR addresses a number of issues in some gradle tasks:
   - ignores empty directories for task inputs
   - configures task dependencies
   - replace relative paths with absolute paths where applicable
   - enable build caching for tasks that did not support it but should
   
   ## Motivation and Context
   Some gradle tasks were configured in a way that prevented incremental builds and build caching from being used effectively. This PR addresses some of these issues. 
   
   ## How Has This Been Tested?
   This has been tested with the [Gradle Enterprise build validation scripts](https://github.com/gradle/gradle-enterprise-build-validation-scripts/blob/main/Gradle.md).
   
   In the build scans below you can see that build cache statistics have improved between master and this PR:
   - [second run on master](https://scans.gradle.com/s/oe44oq4zgihym/performance/execution)
   - [second run on PR](https://scans.gradle.com/s/anhmnqs35j3sw/performance/execution)
   
   ## Screenshots (if appropriate):
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Delete as appropriate -->
   - Non-breaking improvements to the build
   
   ## Checklist:
   - [x] My code follows the [code style][style-guide] of this project.
   - [ ] I have updated the documentation accordingly.
   
   [style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
   


-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#issuecomment-1376765607

   I've filed a follow-up for adding warning in Gradle: https://github.com/gradle/gradle/issues/23456


-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#issuecomment-1384022139

   I've integrated the changes, thank you.


-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on a diff in pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#discussion_r1066180900


##########
build.gradle.kts:
##########
@@ -98,6 +98,8 @@ val rat by tasks.getting(org.nosphere.apache.rat.RatTask::class) {
     verbose.set(true)
     // Note: patterns are in non-standard syntax for RAT, so we use exclude(..) instead of excludeFile
     exclude(rootDir.resolve(".ratignore").readLines())
+    exclude("src/dist-check/temp")

Review Comment:
   Here's a relevant Gradle issue which, unfortunately, was declined: https://github.com/gradle/gradle/issues/18882
   
   I believe, it would still be nice if Gradle could support `.gitignore` by default since it is quite common to use Gradle + Git together.



-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] sandra-thieme commented on a diff in pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
sandra-thieme commented on code in PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#discussion_r1070928017


##########
build.gradle.kts:
##########
@@ -98,6 +98,8 @@ val rat by tasks.getting(org.nosphere.apache.rat.RatTask::class) {
     verbose.set(true)
     // Note: patterns are in non-standard syntax for RAT, so we use exclude(..) instead of excludeFile
     exclude(rootDir.resolve(".ratignore").readLines())
+    exclude("src/dist-check/temp")
+    dependsOn(":src:dist:copyBinLibs", ":src:dist:copyLibs")

Review Comment:
   This change is intended. It is necessary to support incremental builds for the `rat` task:
   
   ```
   Execution optimizations have been disabled for task ':rat' to ensure correctness due to the following reasons:
   - Gradle detected a problem with the following location: '/Users/sandra/experiments/gradle-enterprise-gradle-build-validation/.data/01-validate-incremental-building/20230105T073724-63b67024/build_jmeter'. Reason: Task ':rat' uses this output of task ':src:dist:copyBinLibs' 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.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
   - Gradle detected a problem with the following location: '/Users/sandra/experiments/gradle-enterprise-gradle-build-validation/.data/01-validate-incremental-building/20230105T073724-63b67024/build_jmeter'. Reason: Task ':rat' uses this output of task ':src:dist:copyLibs' 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.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
   ```



-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#issuecomment-1377736002

   @sandra-thieme , I've integrated all the changes, except `dependsOn(":src:dist:copyBinLibs", ":src:dist:copyLibs")`.
   
   Frankly speaking, I think `dependsOn(":src:dist:copyBinLibs", ":src:dist:copyLibs")` should make no difference since `/lib/**/*.jar` is present in `.gitignore`, and `copyLibs`/`copyBinLibs` populate `/libs` folder.
   


-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#issuecomment-1375314295

   Thanks for the PR. Do you know if Gradle could be configured to produce warning (or even fail the build) when `withPropertyName` and `withPathSensitivity` are missing?


-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi closed pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi closed pull request #5757: Build improvements
URL: https://github.com/apache/jmeter/pull/5757


-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on a diff in pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#discussion_r1066179374


##########
build.gradle.kts:
##########
@@ -98,6 +98,8 @@ val rat by tasks.getting(org.nosphere.apache.rat.RatTask::class) {
     verbose.set(true)
     // Note: patterns are in non-standard syntax for RAT, so we use exclude(..) instead of excludeFile
     exclude(rootDir.resolve(".ratignore").readLines())
+    exclude("src/dist-check/temp")

Review Comment:
   Well, we have support for excluding `.gitignore` patterns, so I would put `src/dist-check/temp` to `.gitignore`, so it would be excluded automatically from all the relevant copy specs.



-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on a diff in pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#discussion_r1064413043


##########
build.gradle.kts:
##########
@@ -98,6 +98,8 @@ val rat by tasks.getting(org.nosphere.apache.rat.RatTask::class) {
     verbose.set(true)
     // Note: patterns are in non-standard syntax for RAT, so we use exclude(..) instead of excludeFile
     exclude(rootDir.resolve(".ratignore").readLines())
+    exclude("src/dist-check/temp")
+    dependsOn(":src:dist:copyBinLibs", ":src:dist:copyLibs")

Review Comment:
   I guess this is not intended. `rat` task should verify the license headers for the files present under source control, so there's no sense in `dependsOn(...copyBinLibs)`. Would you clarify what was the warning that required `dependsOn`?



-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on a diff in pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#discussion_r1064414602


##########
src/licenses/build.gradle.kts:
##########
@@ -51,7 +51,7 @@ fun gradleWrapperVersion(wrapperProps: String) =
 
 val gatherSourceLicenses by tasks.registering(GatherLicenseTask::class) {
     val wrapperProps = "$rootDir/gradle/wrapper/gradle-wrapper.properties"
-    inputs.file(wrapperProps)
+    inputs.file(wrapperProps).withPathSensitivity(PathSensitivity.RELATIVE).withPropertyName("wrapper.props")

Review Comment:
   ```suggestion
       inputs.file(wrapperProps).withPathSensitivity(PathSensitivity.NONE).withPropertyName("wrapper.props")
   ```



-- 
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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [jmeter] vlsi commented on a diff in pull request #5757: Build improvements

Posted by GitBox <gi...@apache.org>.
vlsi commented on code in PR #5757:
URL: https://github.com/apache/jmeter/pull/5757#discussion_r1071213108


##########
build.gradle.kts:
##########
@@ -98,6 +98,8 @@ val rat by tasks.getting(org.nosphere.apache.rat.RatTask::class) {
     verbose.set(true)
     // Note: patterns are in non-standard syntax for RAT, so we use exclude(..) instead of excludeFile
     exclude(rootDir.resolve(".ratignore").readLines())
+    exclude("src/dist-check/temp")
+    dependsOn(":src:dist:copyBinLibs", ":src:dist:copyLibs")

Review Comment:
   That is false positive.
   `rat` ignores `lib/**/*.jar` while `copyLibs` copies only `*.jar` files.
   
   I'm inlined to add `mustRunAfter` to silence Gradle 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: dev-unsubscribe@jmeter.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org