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.

Sign up for GitHub

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

Jump to bottom

Introduce a Gradle inspector #6782

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Introduce a Gradle inspector #6782

merged 5 commits into from
Apr 4, 2023

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch from 56e44b5 to 43fc935 Compare March 31, 2023 08:34
@codecov
Copy link

codecov bot commented Mar 31, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (f619841) 64.62% compared to head (3448b81) 64.62%.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #6782   +/-   ##
=========================================
  Coverage     64.62%   64.62%           
  Complexity     1955     1955           
=========================================
  Files           322      322           
  Lines         16166    16166           
  Branches       2296     2296           
=========================================
  Hits          10448    10448           
  Misses         4726     4726           
  Partials        992      992
Flag Coverage Δ
funTest-docker 63.44% <ø> (-0.88%) ⬇️
funTest-non-docker 49.48% <ø> (ø)
test 32.31% <ø> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch 6 times, most recently from bb11b17 to 3b2c3ae Compare March 31, 2023 13:45
@sschuberth sschuberth mentioned this pull request Mar 31, 2023
Closed
@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch from 3b2c3ae to b550764 Compare March 31, 2023 14:01
@sschuberth
fix(PackageManager): Default to only enabled managers
bcd9f85 
This is a fixup for 38db2a5.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch from b550764 to 909400a Compare March 31, 2023 14:50
@sschuberth sschuberth marked this pull request as ready for review March 31, 2023 20:26
@sschuberth sschuberth requested review from a team as code owners March 31, 2023 20:26
@sschuberth
Copy link
Member Author

PS: There are no tests in here, as my original goal was to reuse the existing Gradle tests, but that turned out to be more complicated than I thought. Partly, because the GradleInspector yields more complete results.

@sschuberth sschuberth mentioned this pull request Apr 2, 2023
Closed
@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch from 909400a to a75e54c Compare April 2, 2023 20:05
@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch from a75e54c to c96d8b4 Compare April 3, 2023 05:52
@sschuberth
feat(package-managers): Add beginnings of a Gradle inspector
9a9e665 
Start a novel approach based on a stand-alone Gradle plugin (written in
Kotlin) to analyze Gradle projects. The goal is to address shortcomings of
the current implementation:

- The `init.gradle` script cannot be debugged (only `buildSrc` and
  stand-alone plugins can [1]).

- The dynamically typed Groovy code is hard to maintain.

- Analysis of modern Android apps throws exceptions due to ambiguous
  configuration / variant selection.

- Binary artifacts are unnecessarily resolved.

- Maven is used to resolve metadata about artifacts, imitating Gradle's
  resolution logic / repositories to query.

At this stage the analysis only builds up the dependency tree to limit
the complexity of this initial implementation. Parsing of package
metadata will be added in follow-up changes.

Also, the new `GradleInspector` plugin is disabled by default for now to
not interfere with the existing `Gradle` plugin.

Resolves #6158.

[1]: https://docs.gradle.org/current/userguide/troubleshooting.html#sec:troubleshooting_build_logic

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
feat(gradle-inspector): Parse package metadata from Maven POMs
65b2f91 
Explicitly resolve parent POMs to ensure they are available as XML files
in the Gradle cache. Based on that, build the effective POMs for all
dependencies in order to parse package metadata from them. As the POMs are
retrieved from the Gradle cache, no download via Maven is involved.

Note that the Maven-model-specific code is mostly based on code from
`MavenSupport`, adjusted to work with slightly different classes.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
feat(gradle-inspector): Create remote artifacts for packages
60ff668 
Create remote artifacts based on the POM URLs. The hashes for the
artifacts are retrieved from the respective accompanying text files,
without downloading the artifact itself.

Note that this approach will fail for private Maven repository even if
Gradle is configured with the credentials, as this download happens
directly via OkHttp.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
feat(OrtModelBuilder): Ignore Android dependencies metadata configura…
3448b81 
…tions

For these configurations often Gradle itself has resolution failures. As
their sole purpose is to describe the dependencies that are compiled into
an Android app, just ignore them as that is what ORT itself analyzes.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the gradle-inspector-model-builder branch from c96d8b4 to 3448b81 Compare April 3, 2023 10:13
@sschuberth sschuberth merged commit 3a80f47 into main Apr 4, 2023
@sschuberth sschuberth deleted the gradle-inspector-model-builder branch April 4, 2023 07:53
fviernau
fviernau requested changes Apr 4, 2023
View reviewed changes
plugins/package-managers/gradle-inspector/README.md Show resolved Hide resolved
plugins/package-managers/gradle-inspector/README.md Show resolved Hide resolved
plugins/package-managers/gradle-inspector/README.md Show resolved Hide resolved
plugins/package-managers/gradle-inspector/src/main/kotlin/GradleInspector.kt Show resolved Hide resolved
plugins/package-managers/gradle-inspector/src/main/kotlin/GradleInspector.kt Show resolved Hide resolved
plugins/package-managers/gradle-inspector/src/main/kotlin/GradleInspector.kt Show resolved Hide resolved
plugins/package-managers/gradle-plugin/src/main/kotlin/OrtModelBuilder.kt Show resolved Hide resolved
plugins/package-managers/gradle-plugin/src/main/kotlin/OrtModelBuilder.kt
val isDeprecatedConfiguration = GradleVersion.current() >= GradleVersion.version("6.0")
&& this is DeprecatableConfiguration && resolutionAlternatives != null

return canBeResolved && !isDeprecatedConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Am I right with assuming that a deprecated configuration is not resolvable?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, deprecated configurations are resolvable. That's why they need to be filtered out explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

I got this hint from the function name, do you agree function name is mis-leading?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I can see how to miss the notion between "canBeResolved" and "isResolvable". I'm open for proposals for better names for the extension function. Maybe "shouldResolve", "isAccepted" or "isEligible"?

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 think I'll go for "isRelevent" for now, also in context of a minor refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, my only idea is shouldBeIgnored() which I'm not sure if I like.
Maybe separate functions could do. project.configurations.filter { it.isResolvable() && !it.isDeprecated() }

I believe shouldResolve() would make it clearer to me compared to the current code.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe while at it, a comment could be added explaining why deprecated configurations are ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

why deprecated configurations are ignored.

Isn't that obvious? Because the are deprecated (in favor of another configuration).

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that obvious? Because the are deprecated (in favor of another configuration).

Does the deprecation imply that the build script is not using that configuration to define it's dependencies?
Or could the build script still use deprecated configs?

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 cannot answer these questions fully. In any case, please note that ignoring deprecated configurations with alternatives is already done in the "legacy" Gradle analyzer (and was not introduced by myself), so it's not a new thing.

To my understanding deprecated configurations are a thing Gradle introduced mainly for itself (i.e. not for authors of build files) to deprecated its own "compile" and "runtime" configurations, see gradle/gradle#8585. Deprecated configurations without alternatives would still get resolved, but deprecated configurations with alternatives would not get resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fviernau fviernau fviernau requested changes

@mnonnenmacher mnonnenmacher mnonnenmacher approved these changes

@oheger-bosch oheger-bosch Awaiting requested review from oheger-bosch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sschuberth @mnonnenmacher @fviernau @oheger-bosch @MarcelBochtler