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

#1114 print build path in projectHealth #1178

Merged
merged 1 commit into from May 21, 2024

Conversation

seregamorph
Copy link
Contributor

@seregamorph seregamorph commented Apr 27, 2024

Solves #1114 (only projectHealth, but not buildHealth).
Prints absolute path of according build.gradle of the project module. This can be pretty helpful in large multi-module projects, especially when there is no full matching between module name and submodule directory.

Sample task output:

> Task :utilities:projectHealth
/Users/morph/Projects/demo-gradle-multi-module/utilities/build.gradle
Unused dependencies which should be removed:
  testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'

Using absolute path

The absolute path is printed intentionally as it seems to be the best trade-off.
This path is recognized in IDEA to open file in "Navigate to File...":
Screenshot 2024-04-27 at 19 25 50
For instance, if relative path is used, there can be ambiguity:
Screenshot 2024-04-27 at 19 26 10
Also, the relative path is not always in parent directory (for included builds) and printing "../../parent/build.gradle" will not help navigating to the file in the IDE.

@seregamorph seregamorph marked this pull request as ready for review April 27, 2024 19:29
@seregamorph
Copy link
Contributor Author

I've updated the implementation moving the gradle file name prepending to ProjectHealthTask not to cache the GenerateProjectHealthReportTask output with absolute path (while it's not a task input).

Copy link
Owner

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! I only worry about it being inconsistent with how buildHealth works, while simultaneously being concerned about how verbose it would be if buildHealth also had this behavior. But I'm happy to support this use-case, for the reasons you indicated.

@@ -15,6 +15,8 @@ import org.gradle.api.tasks.TaskAction

abstract class ProjectHealthTask : DefaultTask() {

private val buildFilePath = project.buildFile.path
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this should be an instance property. You can just move this down into the task action.

Oh, and of course I think that means that either buildFile or buildFile.path must then be a task input property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The project (getProject()) cannot be executed inside of @TaskAction for tasks that are not explicitly marked as non-cacheable:

1 problem was found storing the configuration cache.
- Task `:proj:projectHealth` of type `com.autonomousapps.tasks.ProjectHealthTask`: invocation of 'Task.project' at execution time is unsupported.
  See https://docs.gradle.org/7.4/userguide/configuration_cache.html#config_cache:requirements:use_project_during_execution

So this simple trick is used to resolve the project parameter on the phase before TaskAction execution and not to pass as Input from the plugin. You suggested to make it an input - Done. Initially I didn't mark it as input, because this task is not cacheable.

Comment on lines 45 to 50
private fun prependBuildPath(consoleReport: String): String {
return buildFilePath + "\n" + consoleReport
}
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer this be inlined.

val output ="${project.buildFile.path}\n$consoleReport

for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 28 to 46
def "projectHealth prints build file path (#gradleVersion)"() {
given:
def project = new AbiGenericsProject(SourceKind.METHOD)
gradleProject = project.gradleProject

when:
var result = build(gradleVersion, gradleProject.rootDir, 'projectHealth')

then:
assertThat(result.output).contains(
"""${gradleProject.rootDir.getPath()}/proj/build.gradle
|Existing dependencies which should be modified to be as indicated:
| api project(':genericsBar') (was implementation)
| api project(':genericsFoo') (was implementation)
|""".stripMargin())

where:
gradleVersion << gradleVersions()
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you instead create a new spec, ProjectHealthSpec, and put this test there? You can continue to use AbiGenericsProject, if you prefer. I realize that we don't have any specs that run projectHealth, so it makes sense to have that encapsulated somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

gradleProject = project.gradleProject

when:
var result = build(gradleVersion, gradleProject.rootDir, 'projectHealth')
Copy link
Owner

Choose a reason for hiding this comment

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

Please use def instead of var for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@seregamorph
Copy link
Contributor Author

I only worry about it being inconsistent with how buildHealth works

What would you prefer - address both projectHealth and buildHealth in scope of a single PR or implement them separately?
The problem here is that these two tasks have different ways to aggregate and format reports.

@autonomousapps autonomousapps merged commit be75f27 into autonomousapps:main May 21, 2024
1 check passed
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.

None yet

2 participants