review fix(MavenLauncher): Improving MavenLauncher dependancy resolution #2112

nharrand · 2018-06-26T15:54:20Z

Based on @tdurieux 's work. (WiP)

Handles cases where a dependancy's version is not not specified in the targeted pom, but instead is defined earlier or in parent pom.
Handles version with weird name
Handles the properties pom.groupId, pom.artifactId, pom.version
This could probably benefit some more testing and/or some refactoring.
Help welcome!

…arent pom, and other corner cases

surli · 2018-06-27T07:40:12Z

is it ready for review ?

tdurieux · 2018-06-27T07:41:21Z

This class needs a major refactoring because it is a real mess.

surli · 2018-06-27T07:43:16Z

OK it's a WIP then, I edited the title.

… invocation

nharrand · 2018-07-12T14:36:49Z

I propose to change MavenLauncher's behavior: Instead of implementing every single rules of maven dependency resolution to build a classpath, I propose to build it by invoking maven.

On the pro side: If maven can run the project, then spoon should at least find the bytecode for every classes used by the project.

On the con side: This requires the user to specify the path to maven home (if it is not a common one). So it might break retro compatibility.

surli · 2018-07-12T14:46:11Z

src/main/java/spoon/MavenLauncher.java

-				LOGGER.log(Level.ERROR, "Unable to read the pom of the dependency:" + dependence.toString(), e);
-			}
-			return dependence;
+	private File getDefaultMavenHome() throws SpoonException {


outch! This method looks really ugly!
Can't you rely on environment variable instead? The M2_HOME was considered as a standard one, but it's not heavily used.

Oh you saw that...Fine, I'll correct it.

surli · 2018-07-12T14:47:54Z

Instead of implementing every single rules of maven dependency resolution to build a classpath

Maven is written in Java. Shouldn't we get the implementation of Maven concerning their dependency resolution?

nharrand · 2018-07-12T14:50:49Z

I agree, but I could not find the documentation to do that. I'll look better.

surli · 2018-07-12T14:51:22Z

It's maven: the documentation is the source ;) https://github.com/apache/maven-dependency-plugin/tree/maven-dependency-plugin-3.1.1

nharrand · 2018-07-12T21:18:30Z

I've tried to implement a version that uses directly MavenCli programatically (No calls to MavenInvoker). But to be honest, I am unsure whether it is a good idea.
It relies on maven packages that I cannot declare (or at least did not succeed to) in the pom because it generates some version conflict (as far as I understand).
I think that currently, it works on my machine, because it happens that in my classpath the maven classes follow an api that is the same as the one I expect, but I am unsure if it will work on a machine with a different version of maven installed, or if spoon is used in other circumstances.

I am starting to think that there is a reason why most people seem to use MavenInvoker. I think it is more stable because it isolate the nested call to maven.

What do you think? (Am I missing something?)

surli · 2018-07-13T08:01:07Z

So if I sum up: the problem is Maven dependencies are difficult to resolve, so the good way to do it is to use Maven directly.

One solution is then to use a Maven Invoker, but clients would have to specify their Maven path.
Another solution is to use Maven dependency resolver but then we cannot guarantee a good behaviour, depending on the version of Maven the clients use.

Then yeah maybe it makes sense to use your first solution...
I propose that you change the API:

by default MavenLauncher checks for the env var M2_HOME and try to use it
another constructor is created with an argument for the Maven executable path
if M2_HOME is not available and the argument is not used an expection is thrown

WDYT?

nharrand · 2018-07-18T09:36:18Z

One solution is then to use a Maven Invoker, but clients would have to specify their Maven path.
Another solution is to use Maven dependency resolver but then we cannot guarantee a good behaviour, depending on the version of Maven the clients use.

Yes, that is at least how I see it.

by default MavenLauncher checks for the env var M2_HOME and try to use it
another constructor is created with an argument for the Maven executable path
if M2_HOME is not available and the argument is not used an expection is thrown

I think we could also try to exec directly the command mvn -version to determine the path to maven home, before throwing an error.

nharrand · 2018-07-19T12:17:28Z

Could someone who has access to travis CI add the following environment variable M2_HOME="/usr/local/maven" ? (If I understand correctly I can't change that from the configuration file.)

nharrand · 2018-08-01T18:15:39Z

This code is ready for review!
(Please don't judge the last ten commits of struggle ending up on on just adding -c in one travis script...)

surli · 2018-08-02T07:39:12Z

chore/travis/travis-jdk10.sh

@@ -8,6 +8,6 @@ wget https://raw.githubusercontent.com/sormuras/bach/master/install-jdk.sh
 chmod +x install-jdk.sh

 export JAVA_HOME=$HOME/openjdk8
-source ./install-jdk.sh -f 10
+source ./install-jdk.sh -f 10 -c


Could you explain why you needed this argument?

Yes! So basically it tells install-jdk.sh to use the default (the one present in the base conatiner) collection of trusted certificate authority (CA) certificates for java. Without this option, calling maven from java results in SSL errors.

ok, can you add a comment on the script please?

surli · 2018-08-02T07:39:53Z

pom.xml

+    <dependency>
+      <groupId>org.apache.maven.shared</groupId>
+      <artifactId>maven-invoker</artifactId>
+      <version>2.2</version>


So you have one version 3.1.1 of maven-invoker and one version 2.2? Does it make sense?

Probably not. This is a leftover from previous trials. I'll fix this ASAP.

surli · 2018-08-02T07:41:39Z

src/main/java/spoon/MavenLauncher.java

+		properties.setProperty("mdep.outputFile", "spoon.classpath.tmp");
+		request.setProperties(properties);
+
+		//FIXME Should the standard output made silent and error verbose?


It might be interesting to use a logger debug for this one? WDYT?

Ok. I'll add it ASAP.

surli · 2018-08-02T07:43:34Z

src/main/java/spoon/MavenLauncher.java

+	 *
+	 * @param classPathFiles File[] containing the classpath elements separated with ':'
+	 */
+	public static String[] readClassPath(File ... classPathFiles) {


Why does it take an array of files here? You should create only one classpath file, don't you?

So I thought too. But this is the behavior of maven-dependency-plugin for multi module projects. It creates one classpath file per module.

OK it worthes a comment then

Moreover I'm not sure the method should be public here?

For the comment, agreed.
About the public, my idea of it was to expose a way for the user to manually add its classpath. But if you think it is not relevent, I can totally make it private.

for the user to manually add its classpath

You can still use the environment.setSourceClasspath

Yes, but his method gives you what content to feed in environment.setSourceClasspath should you choose to generate it with maven.

Yeah but the purpose of this method is to be called internally to feed automatically the environment. Then you can use it to improve the classpath by adding/removing some libs.
So I'm still not sure there's a strong reason to let this one open.

Ok, I made it private.

surli · 2018-08-02T07:44:19Z

src/main/java/spoon/MavenLauncher.java

-					} catch (NumberFormatException ignore) {
+						br.close();
+					} catch (IOException e) {
+						e.printStackTrace(); //These Exceptions occur after the classpath has been read, so it should not prevent normal execution.


Throw instead a SpoonException maybe?

I can. But do we really want to throw an excepetion for a temporary file that we failed to close? I mean the error happen after reading the information we need.

Actually you can use a try with resources to be sure it's closed at the end.
Something like:

try (br = new BufferedReader(new FileReader(classPathFile));) { ... }

And maybe add the throws IOException to the method itself if it's not public anymore. You'll manage those elsewhere.

Ok this should be fixed now.

surli · 2018-08-02T07:44:36Z

src/main/java/spoon/MavenLauncher.java

-				}
-				if (splitVersion.length > 2) {
+			} catch (Exception e) {
+				e.printStackTrace();


Remove that one.

surli · 2018-08-02T07:45:08Z

src/main/java/spoon/MavenLauncher.java

-						}
-					}
+			while ((line = output.readLine()) != null) {
+				if (line.contains("Maven home: ")) {


Could you use a constant for "Maven home: " please?

Yes, indeed.

surli · 2018-08-02T07:45:46Z

src/main/java/spoon/MavenLauncher.java

-			return dependence;
+			p.waitFor();
+		} catch (IOException e) {
+			e.printStackTrace();


Here again: declaring two exception for the same behaviour is useless. You can catch both in the same block and throw a SpoonException here.

surli · 2018-08-02T07:46:18Z

src/main/java/spoon/MavenLauncher.java

+		try {
+			classPathPrints = Files.find(Paths.get(pom.getParentFile().getAbsolutePath()),
+					Integer.MAX_VALUE,
+					(filePath, fileAttr) -> filePath.endsWith("spoon.classpath.tmp"))


Here again: use a constant for the name of the file.

surli · 2018-08-02T07:46:30Z

src/main/java/spoon/MavenLauncher.java

+					.map(p -> p.toFile())
+					.collect(Collectors.toList());
+		} catch (IOException e) {
+			e.printStackTrace();


No printstacktrace

surli · 2018-08-02T07:48:44Z

src/test/java/spoon/MavenLauncherTest.java


 		// with the tests
 		launcher = new MavenLauncher("./", MavenLauncher.SOURCE_TYPE.ALL_SOURCE);

-		assertEquals(30, launcher.getEnvironment().getSourceClasspath().length);
+		assertEquals(194, launcher.getEnvironment().getSourceClasspath().length);


Could you add a new test to check this new behaviour: you did this PR because on some projects dependency resolution was not working. So could you put pom.xml files that wasn't working previously and that is working now, as a test?

It's not that it used to not work on certain poms but more than some types were not resolved because not existing in the classpath where they could have been if MavenLauncher generated a classpath more similar to Maven.
I don't think it is testable just with a pom. The problem appears when analyzing an AST.
I could add a test that check that for a maven project (may be spoon itself), all types are resolvable, even those defined in dependencies. WDYT?

I could add a test that check that for a maven project (may be spoon itself), all types are resolvable, even those defined in dependencies. WDYT?

Sounds good yes.

…type references point to resolvable types

surli · 2018-08-09T14:13:52Z

thanks @nharrand :)

@nharrand

One test has been added by @nharrand in #2112 to show the usage of the new dependency resolver. However this test consumes too much memory on our CI, on the job used to deploy maven artefacts: > [ERROR] Tests run: 7, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 68.03 s <<< FAILURE! - in spoon.MavenLauncherTest [ERROR] testTypeResolution(spoon.MavenLauncherTest) Time elapsed: 38.801 s <<< ERROR! java.lang.OutOfMemoryError: GC overhead limit exceeded at spoon.MavenLauncherTest.testTypeResolution(MavenLauncherTest.java:25) https://ci.inria.fr/sos/job/Spoon-Snapshot-Deployer/1689/console I propose to ignore the test for now.

nharrand added 5 commits February 21, 2018 15:17

add a test for getRoleInParent / getValueByRole consistence

0ead891

fix: MavenLauncher resolve dependancies which version is defined in p…

819aa1c

…arent pom, and other corner cases

Merge branch 'master' of https://github.com/nharrand/spoon

153be0d

Merge branch 'master' of https://github.com/nharrand/spoon

95d99d3

Merge branch 'master' of https://github.com/nharrand/spoon

991985b

surli changed the title ~~fix(MavenLauncher): Improving MavenLauncher dependancy resolution~~ WIP fix(MavenLauncher): Improving MavenLauncher dependancy resolution Jun 27, 2018

nharrand added 4 commits July 12, 2018 16:16

feat(MavenLauncher): MavenLauncher now get its classpath from a maven…

fa60f93

… invocation

clean(MavenLauncherTest): remove unused import

8fef644

Merge branch 'master' into master

5e03011

Update MavenLauncherTest.java

5a5c2c6

surli reviewed Jul 12, 2018

View reviewed changes

nharrand added 3 commits July 12, 2018 23:09

POC MavenLauncher implementation with MavenCli

c7ed307

POC MavenLauncher implementation with MavenCli

9729941

merge

912325d

cleanup(MavenLauncherTest)

7a956fe

nharrand added 4 commits July 18, 2018 13:28

refactor(MavenLauncher): use MavenInvoker

9d74873

merge

869d445

set up M2_HOME environement variable in travis

78b5aab

revert previous travis changes

98a84d9

nharrand added 9 commits July 26, 2018 11:29

debugging travis script jdk10

b3427ea

refactoring, update certificate on travis script for java 10

fb5c7aa

stop deleting files before reading them...

eb54db8

yet another attempt at fixing travis build with java 10

75fbf98

yet another attempt at fixing travis build with java 10

e889890

switch to multi classpath files

6006a54

merge and checkstyle fixes

2f8f42c

this certificate problem is getting annoying

54befff

fail first

51b9200

surli changed the title ~~WIP fix(MavenLauncher): Improving MavenLauncher dependancy resolution~~ review fix(MavenLauncher): Improving MavenLauncher dependancy resolution Aug 2, 2018

surli reviewed Aug 2, 2018

View reviewed changes

nharrand added 2 commits August 2, 2018 12:18

Post review improvements

e880745

fix some exception handeling, add a reflexive test to check that all …

57af73c

…type references point to resolvable types

surli merged commit eed3e53 into INRIA:master Aug 9, 2018

surli mentioned this pull request Aug 9, 2018

fix(MavenLauncher): regression with MavenLauncher on windows #2358

Merged

surli mentioned this pull request Aug 10, 2018

review fix: ignore a test for memory issues on the CI #2366

Merged

monperrus mentioned this pull request Sep 20, 2018

Release of Spoon 7.1.0 #2489

Closed

4 tasks

review fix(MavenLauncher): Improving MavenLauncher dependancy resolution #2112

review fix(MavenLauncher): Improving MavenLauncher dependancy resolution #2112

Conversation

nharrand commented Jun 26, 2018

surli commented Jun 27, 2018

tdurieux commented Jun 27, 2018

surli commented Jun 27, 2018

nharrand commented Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surli commented Jul 12, 2018

nharrand commented Jul 12, 2018 • edited Loading

surli commented Jul 12, 2018

nharrand commented Jul 12, 2018 • edited Loading

surli commented Jul 13, 2018

nharrand commented Jul 18, 2018

nharrand commented Jul 19, 2018 • edited Loading

nharrand commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharrand Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharrand Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surli commented Aug 9, 2018

nharrand commented Jul 12, 2018 •

edited

Loading

nharrand commented Jul 12, 2018 •

edited

Loading

nharrand commented Jul 12, 2018 •

edited

Loading

nharrand commented Jul 19, 2018 •

edited

Loading

nharrand Aug 2, 2018 •

edited

Loading

nharrand Aug 2, 2018 •

edited

Loading