-
Notifications
You must be signed in to change notification settings - Fork 171
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
Gradle support #74
Gradle support #74
Conversation
if err == nil { | ||
builder.GradleCmd = gradleCmd | ||
|
||
gradleVersionMatchRe := regexp.MustCompile(`Gradle ([0-9]+\.[0-9]+.\w+)`) |
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.
It's probably more useful (for debugging purposes) to just capture the entire --version
output rather than checking for a version.
builders/gradle.go
Outdated
|
||
// TODO: We need to let the user configure the right configurations | ||
dependenciesOutput, err := exec.Command(builder.GradleCmd, m.Name+":dependencies", "-q", "--configuration=compile", "--offline", "-a").Output() | ||
// Do not handle error, as gradle will exit with status 1 regardless |
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.
I'm not sure what this comment means -- aren't you handling err
just below?
builders/gradle.go
Outdated
} | ||
|
||
var deps []module.Dependency | ||
dependenciesRe := regexp.MustCompile(`- ([\w\.-]+):([\w\.-]+):([\w\.-]+)( -> ([\w\.-]+))?`) |
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 regex is unmaintainable without a very detailed comment. I think it would be much cleaner to loop through each line and check for ---
instead (which prefixes all lines with dependency names).
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.
That's not true, since it renders as a tree the parent nodes have a different prefix. There is no way to guarantee a specific format for each level as well.
The only common prefix i found was -
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.
Really? I see all nodes being prefixed with either \---
or +---
. Is this because we're using different versions? Do you have an example project where this isn't true?
builders/gradle.go
Outdated
if err := builder.Initialize(); err != nil { | ||
return nil, err | ||
} | ||
// Look for the root Gradle build |
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.
These comments are out of date.
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.
How so?
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.
We're parsing gradle tasks
output instead of looking for *.gradle
files now. I might have been looking at an older commit.
builders/gradle.go
Outdated
trimmed := strings.TrimSpace(line) | ||
if len(trimmed) > 0 { | ||
depMatchIndicies := taskListRe.FindStringIndex(trimmed) | ||
if len(depMatchIndicies) > 0 && depMatchIndicies[0] == 0 { |
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.
*Indices
LGTM, modulo nits. @xizhao, can you LGTM this PR when ready? I opened it so it won't let me LGTM. |
Gradle still needs the build cmd, it's missing APIs |
Only run IPR scan if all of the IPR options are provided
Follow-up work: