add feature: support resolving pom.xml for maven #50

MoGuGuai-hzr · 2021-07-25T16:59:23Z

Solution: copy, using mvn dependency:copy-dependencies, all dependencies and transitive dependencies, and then resolve license from the jar files.

Known disadvantages:

depends on maven
replication of the jar files needs to read and write the disk

wu-sheng

One question, this PR is only including dependency resolving, but not checking the dependencies license compatible, right?

pkg/deps/maven_test.go

kezhenxu94 · 2021-07-25T23:40:15Z

One question, this PR is only including dependency resolving, but not checking the dependencies license compatible, right?

All the dependencies licenses resolvers including Go, NPM don’t check license compatibility now, they just resolve the licenses for human’s reference. Checking license compatibility is out of the scope of this PR and will be discussed after the main resolvers (Java, NodeJS, Golang) can correctly resolve the licenses first.

wu-sheng

This and #48 should be very similar PR, but why do the changes have so many differences?

Resolvers is not changed here.
.licensserc.yaml is not changed in Add support for resolving npm dependencies' licenses #48

.licenserc.yaml

pkg/deps/maven.go

kezhenxu94 · 2021-07-26T00:07:15Z

pkg/deps/maven.go

+	var err error
+	_, err = exec.Command("mvn", "--version").Output()
+	if err != nil {
+		return
+	}
+
+	Resolvers = append(Resolvers, new(MavenPomResolver))


I think this is not a good condition to check whether we should add the resolver:

mvn version is not enough to determine whether maven resolver can be use, maven wrapper is also widely used in some projects.

We should log some warning / error logs when users' environment don't have maven installed, instead of failing silently.

My suggestion is

just add the resolver to

skywalking-eyes/pkg/deps/resolve.go

Lines 29 to 32 in d6e232b

var Resolvers = []Resolver{

new(GoModResolver),

new(NpmResolver),

}

check whether mvnw exists in the same directory as pom.xml first, if yes, use mvnw, otherwise, use mvn

run the needed command (mvn copy-dependencies), if it failed, we also fail the command and print some logs

kezhenxu94 · 2021-07-27T01:44:07Z

pkg/deps/maven.go

+	err := CheckMVN()
+	if err != nil {
+		logger.Log.Errorln("an error occurred when checking maven tool:", err)
+	}


Let's invoke this in maven resolver manually instead of in init method, so that when users don't need to resolve pom.xml files won't see these logs

have moved it~

kezhenxu94 · 2021-07-27T04:02:04Z

So many changed files / lines 😱

MoGuGuai-hzr · 2021-07-27T04:25:21Z

So many changed files / lines 😱

It may be caused by the CR/LF, now it is fixed.

kezhenxu94 · 2021-07-27T07:54:08Z

pkg/deps/maven.go

+	err := resolver.CheckMVN()
+	if err != nil {
+		logger.Log.Errorln("an error occurred when checking maven tool:", err)
+		return false
+	}


Please bear in mind, CanResolve will be invoked for every kind of projects, not just maven projects, so if I just want to resolve Go project or NPM project, this error logs look weird to me. Note that I'm not saying we should not log this error, instead, we should only check mvn right before when we need to run mvn command, (in the Resolve method), CanResolve is only used to filter the resolver according to the files.

kezhenxu94 · 2021-07-27T07:56:36Z

pkg/deps/maven.go

+	if err := os.Chdir(dir); err != nil {
+		logger.Log.Errorf("an error occurred when entering dir <%s> to parser file <%s>:%v\n", dir, base, err)
+		return false
+	}


I think this part should be moved into CheckMVN function?

kezhenxu94 · 2021-07-27T07:58:59Z

pkg/deps/maven.go

+	_, err = exec.Command("./mvnw", "--version").Output()
+	if err == nil {
+		resolver.maven = "./mvnw"
+		logger.Log.Debugln("use mvnw")


mvnw is found, will use mvnw by default

does this mean to modify the log message?

does this mean to modify the log message?

Uh yes, sorry, didn't make it clear 😢

it is my fault, I didn't understand it well😅

MoGuGuai-hzr · 2021-07-30T09:29:58Z

added some patterns describing the possible locations of license, but does not work for some jar packages

MoGuGuai-hzr

should i crawl the original webpage or judge directly from the url to identify the license

kezhenxu94 · 2021-07-30T13:48:47Z

should i crawl the original webpage or judge directly from the url to identify the license

We can consider this later, we don't need to do all in one PR (which is hard to review and cost too long to finish), we can deliver minimal yet complete features step by step, in this PR I think it's OK to only support the "standard" dependencies that have a LICENSE file in their .jar, so let's polish this part instead of keep adding new things in this PR

MoGuGuai-hzr · 2021-07-30T14:33:51Z

should i crawl the original webpage or judge directly from the url to identify the license

We can consider this later, we don't need to do all in one PR (which is hard to review and cost too long to finish), we can deliver minimal yet complete features step by step, in this PR I think it's OK to only support the "standard" dependencies that have a LICENSE file in their .jar, so let's polish this part instead of keep adding new things in this PR

ok~, i will do it

kezhenxu94 · 2021-08-02T02:04:24Z

pkg/deps/maven.go

+		return fmt.Errorf("neither found mvnw nor mvn")
+	}
+
+	logger.Log.Debugln("mvnw is not found, will use mvn by default")


This is not needed / correct now,

ok, i have removed it

kezhenxu94 · 2021-08-02T02:47:00Z

pkg/deps/maven.go

+}
+
+func (resolver *MavenPomResolver) DownloadDeps() error {
+	cmd := exec.Command(resolver.maven, "process-resources") // #nosec G204


This seems to be problematic in multi-modules maven project, can you try this feature in our main repo http://github.com/apache/skywalking ?

as you said in discussion, the failure may be caused by submodules. so i resolve it using maven command install -DskipTests to install possible need dependencies, and it may take some time.

wu-sheng · 2021-08-03T01:56:03Z

Just a reminder, I am not sure whether this could be fixed easily. Maven includes a plugin called shaded, which would not include the dependency jar, but package it inside another package during the install. You could try this case.

MoGuGuai-hzr · 2021-08-03T02:05:46Z

Just a reminder, I am not sure whether this could be fixed easily. Maven includes a plugin called shaded, which would not include the dependency jar, but package it inside another package during the install. You could try this case.

thanks for your help, and i am using install command attempting to resolve the incompatibility

wu-sheng · 2021-08-03T02:17:07Z

Just a reminder, I am not sure whether this could be fixed easily. Maven includes a plugin called shaded, which would not include the dependency jar, but package it inside another package during the install. You could try this case.

thanks for your help, and i am using install command attempting to resolve the incompatibility

My point is, the module using this plugin, may escape from your dependency check. You should check the case.

MoGuGuai-hzr · 2021-08-03T02:33:12Z

Just a reminder, I am not sure whether this could be fixed easily. Maven includes a plugin called shaded, which would not include the dependency jar, but package it inside another package during the install. You could try this case.

thanks for your help, and i am using install command attempting to resolve the incompatibility

My point is, the module using this plugin, may escape from your dependency check. You should check the case.

sorry, I didn’t understand it very well. does this mean giving up on resolving these module? since the shaded package will be packaged during the install, can it be solved by installing first?

wu-sheng · 2021-08-03T02:44:07Z

orry, I didn’t understand it very well. does this mean giving up on resolving these module? since the shaded package will be packaged during the install, can it be solved by installing first?

I don't know. This is only a reminder, I just recommend you to check.

MoGuGuai-hzr · 2021-08-03T02:49:11Z

orry, I didn’t understand it very well. does this mean giving up on resolving these module? since the shaded package will be packaged during the install, can it be solved by installing first?

I don't know. This is only a reminder, I just recommend you to check.

ok, I've got the information. i will attemp it later

kezhenxu94

Nice work! This is closing to serve as the 1st version for maven projects, though we may have some corner cases to consider later. 👍🏻

I left some comments, please take a look

kezhenxu94 · 2021-08-03T03:50:58Z

pkg/license/identifier.go

+var seemLicense = regexp.MustCompile(`(?i)licen[sc]e|copyright|copying`)
+
+// Seem determine whether the content of the file may be a license file
+func Seem(content string) bool {
+	return seemLicense.MatchString(content)
+}


Can you reuse possibleLicenseFileName in file golang.go? it may not be a good idea to put possibleLicenseFileName in identifier.go but now let's unify the 2 places, you can move possibleLicenseFileName into identifier.go later.

Can you reuse possibleLicenseFileName in file golang.go? it may not be a good idea to put possibleLicenseFileName in identifier.go but now let's unify the 2 places, you can move possibleLicenseFileName into identifier.go later.

the purpose of Seem function was to simply determine whether one file may be a license base on the input content, not the file name. i remove it from identifier.go now.

pkg/deps/maven_test.go

kezhenxu94

Thank you @MoGuGuai-hzr for the great work and patience 🙇🏻

kezhenxu94 self-requested a review July 25, 2021 23:16

kezhenxu94 added the feature label Jul 25, 2021

kezhenxu94 added this to the 0.2.0 milestone Jul 25, 2021

wu-sheng reviewed Jul 25, 2021

View reviewed changes

pkg/deps/maven_test.go Show resolved Hide resolved

wu-sheng reviewed Jul 25, 2021

View reviewed changes

kezhenxu94 reviewed Jul 26, 2021

View reviewed changes

MoGuGuai-hzr force-pushed the tony branch from 8944a3a to 6b07bd8 Compare July 26, 2021 09:39

kezhenxu94 reviewed Jul 27, 2021

View reviewed changes

MoGuGuai-hzr force-pushed the tony branch from 4c191b9 to ebd1d36 Compare July 27, 2021 04:20

kezhenxu94 reviewed Jul 27, 2021

View reviewed changes

MoGuGuai-hzr commented Jul 30, 2021

View reviewed changes

MoGuGuai-hzr force-pushed the tony branch from 77ea669 to 6adebf9 Compare July 30, 2021 14:31

kezhenxu94 reviewed Aug 2, 2021

View reviewed changes

kezhenxu94 previously approved these changes Aug 3, 2021

View reviewed changes

MoGuGuai-hzr dismissed kezhenxu94’s stale review via 22330bf August 3, 2021 06:04

add feature: support resolving pom.xml for maven

65a6b57

MoGuGuai-hzr added 8 commits August 3, 2021 19:04

modify the test case

48c1f71

move the maven check out of init()

c72fd15

added some patterns describing the possible locations of license

d7700e9

fix-lint

c2542b3

fotmat

865152f

use maven install command to resolve dependencies of submodule

429a0d8

skip checkstyle, rat and test

d0b90d4

make CI status passed

87ac61c

MoGuGuai-hzr force-pushed the tony branch from 22330bf to 87ac61c Compare August 3, 2021 11:06

kezhenxu94 approved these changes Aug 3, 2021

View reviewed changes

kezhenxu94 merged commit a952c24 into apache:main Aug 3, 2021

MoGuGuai-hzr deleted the tony branch August 4, 2021 03:18

kezhenxu94 mentioned this pull request Aug 24, 2021

[License-Eye] resolve maven dependencies' licenses apache/skywalking#7311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add feature: support resolving pom.xml for maven #50

add feature: support resolving pom.xml for maven #50

MoGuGuai-hzr commented Jul 25, 2021

wu-sheng left a comment

kezhenxu94 commented Jul 25, 2021

wu-sheng left a comment

kezhenxu94 Jul 26, 2021

kezhenxu94 Jul 27, 2021

MoGuGuai-hzr Jul 27, 2021

kezhenxu94 commented Jul 27, 2021 •

edited

Loading

MoGuGuai-hzr commented Jul 27, 2021

kezhenxu94 Jul 27, 2021

MoGuGuai-hzr Jul 27, 2021

kezhenxu94 Jul 27, 2021

MoGuGuai-hzr Jul 27, 2021

kezhenxu94 Jul 27, 2021 •

edited

Loading

MoGuGuai-hzr Jul 27, 2021

kezhenxu94 Jul 27, 2021

MoGuGuai-hzr Jul 27, 2021

MoGuGuai-hzr commented Jul 30, 2021

MoGuGuai-hzr left a comment

kezhenxu94 commented Jul 30, 2021

MoGuGuai-hzr commented Jul 30, 2021

kezhenxu94 Aug 2, 2021

MoGuGuai-hzr Aug 3, 2021

kezhenxu94 Aug 2, 2021

MoGuGuai-hzr Aug 3, 2021 •

edited

Loading

wu-sheng commented Aug 3, 2021

MoGuGuai-hzr commented Aug 3, 2021

wu-sheng commented Aug 3, 2021

MoGuGuai-hzr commented Aug 3, 2021

wu-sheng commented Aug 3, 2021

MoGuGuai-hzr commented Aug 3, 2021

kezhenxu94 left a comment

kezhenxu94 Aug 3, 2021

MoGuGuai-hzr Aug 3, 2021

kezhenxu94 left a comment

	var Resolvers = []Resolver{
	new(GoModResolver),
	new(NpmResolver),
	}

add feature: support resolving pom.xml for maven #50

add feature: support resolving pom.xml for maven #50

Conversation

MoGuGuai-hzr commented Jul 25, 2021

wu-sheng left a comment

Choose a reason for hiding this comment

kezhenxu94 commented Jul 25, 2021

wu-sheng left a 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

kezhenxu94 commented Jul 27, 2021 • edited Loading

MoGuGuai-hzr commented Jul 27, 2021

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

kezhenxu94 Jul 27, 2021 • 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

MoGuGuai-hzr commented Jul 30, 2021

MoGuGuai-hzr left a comment

Choose a reason for hiding this comment

kezhenxu94 commented Jul 30, 2021

MoGuGuai-hzr commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoGuGuai-hzr Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

wu-sheng commented Aug 3, 2021

MoGuGuai-hzr commented Aug 3, 2021

wu-sheng commented Aug 3, 2021

MoGuGuai-hzr commented Aug 3, 2021

wu-sheng commented Aug 3, 2021

MoGuGuai-hzr commented Aug 3, 2021

kezhenxu94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kezhenxu94 left a comment

Choose a reason for hiding this comment

kezhenxu94 commented Jul 27, 2021 •

edited

Loading

kezhenxu94 Jul 27, 2021 •

edited

Loading

MoGuGuai-hzr Aug 3, 2021 •

edited

Loading