-
Notifications
You must be signed in to change notification settings - Fork 380
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
Handle Maven parent relative path #1149
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<project> | ||
|
||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<groupId>org.upstream</groupId> | ||
<artifactId>wrong-parent</artifactId> | ||
<version>1.1.1</version> | ||
|
||
<packaging>pom</packaging> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<!-- For testing parent relative path --> | ||
<project> | ||
|
||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<groupId>org.test</groupId> | ||
<artifactId>test</artifactId> | ||
<version>1.0.0</version> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,20 +263,22 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project | |
} | ||
visited[current.ProjectKey] = true | ||
var proj maven.Project | ||
if allowLocal && current.RelativePath != "" { | ||
currentPath = filepath.Join(filepath.Dir(currentPath), string(current.RelativePath)) | ||
if filepath.Base(currentPath) != "pom.xml" { | ||
// If the base is not pom.xml, this path is a directory but not a file. | ||
currentPath = filepath.Join(currentPath, "pom.xml") | ||
} | ||
f, err := os.Open(currentPath) | ||
parentFound := false | ||
if parentPath := MavenParentPOMPath(currentPath, string(current.RelativePath)); allowLocal && parentPath != "" { | ||
currentPath = parentPath | ||
f, err := os.Open(parentPath) | ||
if err != nil { | ||
return fmt.Errorf("failed to open parent file %s: %w", current.RelativePath, err) | ||
return fmt.Errorf("failed to open parent file %s: %w", parentPath, err) | ||
} | ||
if err := xml.NewDecoder(f).Decode(&proj); err != nil { | ||
return fmt.Errorf("failed to unmarshal project: %w", err) | ||
} | ||
} else { | ||
if proj.ProjectKey == current.ProjectKey && proj.Packaging == "pom" { | ||
// Only mark parent is found when the identifiers and packaging are exptected. | ||
parentFound = true | ||
} | ||
} | ||
if !parentFound { | ||
// Once we fetch a parent pom.xml from upstream, we should not allow | ||
// parsing parent pom.xml locally anymore. | ||
allowLocal = false | ||
|
@@ -289,6 +291,10 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project | |
// A parent project should only be of "pom" packaging type. | ||
return fmt.Errorf("invalid packaging for parent project %s", proj.Packaging) | ||
} | ||
if proj.ProjectKey != current.ProjectKey { | ||
// The identifiers in parent does not match what we want. | ||
return fmt.Errorf("parent identifiers mismatch: %v, expect %v", proj.ProjectKey, current.ProjectKey) | ||
} | ||
} | ||
// Empty JDK and ActivationOS indicates merging the default profiles. | ||
if err := result.MergeProfiles("", maven.ActivationOS{}); err != nil { | ||
|
@@ -301,6 +307,28 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project | |
return result.Interpolate() | ||
} | ||
|
||
// Maven looks for the parent POM first in 'relativePath', | ||
// then the local repository '../pom.xml', | ||
// and lastly in the remote repo. | ||
func MavenParentPOMPath(currentPath, relativePath string) string { | ||
if relativePath == "" { | ||
relativePath = "../pom.xml" | ||
} | ||
path := filepath.Join(filepath.Dir(currentPath), relativePath) | ||
if info, err := os.Stat(path); err == nil { | ||
if !info.IsDir() { | ||
return path | ||
} | ||
Comment on lines
+319
to
+321
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering what maven does if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is acceptable if the file exists and is a valid parent pom.xml, otherwise, maven tries to download from upstream. |
||
// Current path is a directory, so look for pom.xml in the directory. | ||
path = filepath.Join(path, "pom.xml") | ||
if _, err := os.Stat(path); err == nil { | ||
return path | ||
} | ||
} | ||
|
||
return "" | ||
} | ||
|
||
// For dependencies in profiles and plugins, we use origin to indicate where they are from. | ||
// The origin is in the format prefix@identifier[@postfix] (where @ is the separator): | ||
// - prefix indicates it is from profile or plugin | ||
|
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.
there is some caveat here: we are not able to tell whether the relative path is not set (should default to
../pom.xml
) or set to an empty string (parent should be fetched from upstream).but we still check if the parent pom.xml exists and its key matches what we want, so I think this might be fine...
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.
Maybe there's a change that can be made in the deps.dev repo to set a default value of
relativePath
to../pom.xml
if it's unspecified?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.
this can be investigated - we need to make sure we are able to differentiate the two situations.