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

Initial implementation of (experimental) Dependency API in Gradle #4499

Merged
merged 8 commits into from
Aug 18, 2022

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Aug 12, 2022

Dependency tree is exported from Gradle, using still nonpublic project.dependency API. I admit I need it functioning for the NBLS server, so I propose to go short-term with implementation dependency on project.dependency module. I plan to stabilize and publish the project.dependency as a regular API in NB16 timeframe with ProjectArtifactsQuery that exposes project output artifact(s) for further analysis or processing. I hope I will be able to adapt Maven Graph module to serve for both Maven and Gradle connecting using the new APIs.

Part of this commit is an adjustment to the OCI enterprise feature so it is able to exploit this new Gradle ability to serve dependency tree. During testing it seems that an initial dependency graph of 100 nodes would explode to 10.000+ nodes when converted to a proper tree, so the Dependency API will receive some tweak to allow implementations to compute the nodes lazily and to indicate to the client that two nodes are truly equivalent. In the meantime, I've changed the ADM feature to send a graph instead of tree - it appears working.

Minor fix to Maven implementation.

@sdedic sdedic added API Change [ci] enable extra API related tests Gradle [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests labels Aug 12, 2022
@sdedic sdedic self-assigned this Aug 12, 2022
@sdedic sdedic marked this pull request as ready for review August 12, 2022 10:19
@lkishalmi
Copy link
Contributor

Well, what I am curious is what the additional memory footprint of adding the dependencies into the GradleBaseProject the 10000+ nodes sounds scary.
It would be good to know how essential this information would be. What I'm thinking about, that this info could be fetched/cached in a separate Model. Though that may require more work.

I'm fine to put the dependency info (if it is not that exploded) in the GradleBaseProject.

So it would be good to now what would it put on the IDE on memory and probably on CPU. If it could be expensive let's move it to another model. It can be done later though, if you do not expose the dependencies through GradleBaseProject.

@sdedic
Copy link
Member Author

sdedic commented Aug 13, 2022

@lkishalmi The structure in Configuration is just (currently) just a graph, not an unrolled tree, so in "usual" projects it is rather slim. No ArtifactSpec structures are created in advance and the graph contains target GradleDependency instances, which are created already anyway. The tree is made only when someone asks for the DependencyResult. I'd still think about a way how to indicate to the API client, that a graph node is equivalent to some other (which is the usual case - the subtree can differ if some transitive dependency is excluded, which is not that common), so the client may stop tree traversal.

test.use.jdk.javac=true
spec.version.base=1.19.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we moving the Specification-Version out of manifest.mf and use spec.version.base?
I'm just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://netbeans.apache.org/wiki/DevFaqImplementationDependency.asciidoc -- it's a way how to autoincrease spec version when the implementation dependency changes. I will move it back when I finalize the project.dependency module (based on feedback / findings when implementing Gradle, I'd allow some laziness, still not unsure how).

@@ -0,0 +1,708 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG. Just wanted to add an ANTLR4 Lexer and Parser for simple Groovy based Gradle projects. Anyway, I still have to do that, so this is welcome!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I am working on output artifacts support and for that I'd need some other scanner, at least for simple name=value assignments to various task properties. I plan to supply the list of possible property names (and structure members) from introspecting the tasks / extensions in the project, so the scanner could better ignore 'the unknown' stuff without having to be a full parser.
Stay tuned, it's still in very rough state

@@ -161,7 +166,7 @@ public Map<String, Dependency> getDependencyMap() {

private void buildDependecyMap(Dependency dependency, Map<String, Dependency> result) {
String gav = createGAV(dependency.getArtifact());
result.put(gav, dependency);
result.putIfAbsent(gav, dependency);
dependency.getChildren().forEach((childDependency) -> {
Copy link
Member

Choose a reason for hiding this comment

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

So the line above expect that there could be allredy pair gav : dependency in the result. In this case the dependency is not put again in the result but the children are recountned anyway.

String n = getName();
String g = getGroup();
String v = getVersion();
if (n == null || "".equals(n) || g == null || "".equals(g) || v == null || "".equals(v) || "unspecified".equals(v)) { // NOI18N
Copy link
Member

Choose a reason for hiding this comment

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

Just minor. Using n.isEmpty(), q.isEmtpy() and v.isEmpty would be better here than the equals approach. The null check is done in the condition anyway.

@sdedic sdedic requested a review from lkishalmi August 17, 2022 20:07
@sdedic
Copy link
Member Author

sdedic commented Aug 18, 2022

2 approvals, only DebugRubyTest and DebugBaseTest tests are failing which are unrelated to this PR. Going to merge in a few hours.

@sdedic sdedic merged commit 800a7b8 into apache:master Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change [ci] enable extra API related tests Gradle [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants