Linkage Monitor to ignore Gradle's build directory #1171
Conversation
| @@ -0,0 +1,14 @@ | |||
| <project xmlns="http://maven.apache.org/POM/4.0.0" | |||
There was a problem hiding this comment.
LinkageMonitorTest.testFindLocalArtifacts ensures this file is not picked up.
| String name = pathElement.toString(); | ||
| if (name.equals("build") || name.equals("target")) { | ||
| // Exclude Gradle's build directory and Maven's target directory. | ||
| continue PATHS_LOOP; |
There was a problem hiding this comment.
Is this truly necessary? Usually there's a cleaner approach that does not use continue. Here I think you can reverse the if and avoid continue.
There was a problem hiding this comment.
Removed the label. Does this look good now?
| } | ||
|
|
||
| ImmutableSet<Path> elements = ImmutableSet.copyOf(path); | ||
| if (elements.contains(Paths.get("build")) || elements.contains(Paths.get("target"))) { |
There was a problem hiding this comment.
Does it matter where in the path this directory occurs? Can it be only at the end? What if a something several levels up has the name build or target?
There was a problem hiding this comment.
I would say it does not matter.
From my experience, it's a common practice to avoid using "build" or "target" directory to place something to be published.
There was a problem hiding this comment.
Another logic I have in mind: flags to indicate the existence of "build.gradle" and "pom.xml" files. (It gets complicated as it seeks for accuracy)
There was a problem hiding this comment.
Perhaps we can talk about this offline, but I don't think this one is answered. As written, this seems buggy to me.
|
@elharo As discussed, the exception is on The solution in this PR is just to ignore "build" directory. |
| } | ||
|
|
||
| ImmutableSet<Path> elements = ImmutableSet.copyOf(path); | ||
| if (elements.contains(Paths.get("build")) || elements.contains(Paths.get("target"))) { |
There was a problem hiding this comment.
Perhaps we can talk about this offline, but I don't think this one is answered. As written, this seems buggy to me.
linkage-monitor/README.md
Outdated
|
|
||
| ``` | ||
| set -e # fail if any of command fails | ||
| # Install artifacts to local Maven repository. The command depends on build system of the project. |
| continue; | ||
| } | ||
|
|
||
| Verify.verify( |
There was a problem hiding this comment.
This will throw an exception the calling code isn't prepared to handle. This method should handle both cases here.
There was a problem hiding this comment.
Updated to use path.relativize in case it is an absolute path.
Fixes #1170 . The problem was gax-java's Gradle build directory contains multiple pom.xml files that have the same artifactId.
This change worked fine for a manually-created bad entry in gax-bom: https://gist.github.com/suztomo/85ba6c8f27b40db1809dbdbad8991c85