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

Generate Java call graphs from local JARs in OPALPlugin #459

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

mir-am
Copy link
Contributor

@mir-am mir-am commented May 21, 2022

Description

As discussed in #363, it is now possible to generate call graphs from local JARs in the .m2 folder, created by POMAnalyzer.
This PR makes the following changes:

  • Changes the DataWriter interface to DataRW so that the plugins can read/write from/to the disk given a base directory.
  • OPALPlugin first tries to generate a call graph from a local JAR file if available. If not, it downloads the JAR file from Maven central like before.

Motivation and context

POMAnalyzer stores JAR files to the disk when resolving dependencies. Therefore, it is desirable to generate CGs from local JAR files and also reduce the network activity of OPALPlugin.

Testing

Tested with the DC.

@mir-am mir-am added the enhancement New feature or request label May 21, 2022
@mir-am mir-am self-assigned this May 21, 2022
@mir-am mir-am requested review from ashkboos and proksch May 21, 2022 14:42
Copy link
Contributor

@proksch proksch left a comment

Choose a reason for hiding this comment

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

I think that the solution is sub-optimal... a case distinction that should be taken on a high level is dragged down through the various levels of the API. I have proposed a different strategy in the individual comments.

CHA, date, artifactRepository, ONLY_STATIC_CALLSITES);
long duration = currentTimeMillis() - startTime;
try {
this.graph = OPALPartialCallGraphConstructor.createPCGFromLocalJar(mavenCoordinate, date,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use exception handling for programming logic! Concrete proposal: There is no need to have two separate methods, one createPartialCG method should be enough, which just takes a path to the jar to be analyzed. The only difference is where the path comes from: either mavenCoord.getPath() exists or the .jar is downloaded and a path to the downloaded file is provided. As the jar should always exist locally in our setup, I would be fine with just throwing an error when the file is missing.

* @return RevisionCallGraph of the given coordinate.
* @throws FileNotFoundException
*/
public static PartialJavaCallGraph createPCGFromLocalJar(final MavenCoordinate coordinate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please merge the two createPCG methods into one. There is NO need for a case distinction, both should do exactly the same...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 2c4d825

@mir-am
Copy link
Contributor Author

mir-am commented Sep 9, 2022

@proksch, thanks for the code review and comments.
I have addressed your comments. I admit that this is not the best solution but it leverages our existing JARs in .m2 and reduces the network traffic. Also, it downloads JARs, if not present in .m2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants