-
Notifications
You must be signed in to change notification settings - Fork 314
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
Maven: Switch to the dependency graph format #3894
Maven: Switch to the dependency graph format #3894
Conversation
28eeb67
to
330edf2
Compare
else PackageLinkage.DYNAMIC | ||
|
||
override fun createPackage( | ||
identifier: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to this commit, but what was the reason to use String
instead of Identifier
in the DependencyHandler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I am not entirely sure any more. IIRC, in the original implementation there was no direct 1:1 match between this identifier string and an Identifier
, but this is now indeed the case. There is at least the advantage that the serialization of a plain string takes less space than an Identifier
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, are you planning to change this in this commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this change would affect other functions of the DependencyHandler
interface and the DependencyGraphBuilder
class as well, I would rather think some more about it and do it in a separate PR - if at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat related: #4030
@mnonnenmacher Thanks for the review, I am going to address the points. Just FYI, as discussed in #3825, these changes do not yet have the desired effect to reduce the size of analyzer results. I am therefore working on an approach to share a dependency graph over multiple projects. If this is successful, I would rather base the rework of Maven on this new approach. |
Switched again to draft status to rebase this on the changes introduced in #3913. |
33bb315
to
a22d837
Compare
a22d837
to
1926422
Compare
else PackageLinkage.DYNAMIC | ||
|
||
override fun createPackage( | ||
identifier: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, are you planning to change this in this commit?
This is an implementation of the DependencyHandler interface for the dependency model of Maven. It is going to be used by the Maven package manager to produce results in the optimized DependencyGraph format. Signed-off-by: Oliver Heger <[email protected]>
The ProjectAnalyzerResult returned by the function contained all the packages found by the analyzer. This is not necessarily correct for multi-project builds where different projects can have different dependencies. Fix this issue by filtering the set of packages for the ones that are actually dependencies of the project. Signed-off-by: Oliver Heger <[email protected]>
1926422
to
8bb92f9
Compare
Rework the implementation to use DependencyGraphBuilder together with MavenDependencyHandler to produce the analyzer result. Adapt test classes as necessary. Rather than changing the expected test results, convert results to the classic format before they are compared with the expectations. This check is safer, as it makes sure that the content of the results has not changed, although they are now written in a different format. Add a helper function for this conversion to the test-utils module. Signed-off-by: Oliver Heger <[email protected]>
8bb92f9
to
93e59f9
Compare
This PR is related to #3825. It converts the package manager implementation for Maven to the dependency graph format.