bazelbuild / migration-tooling

Migration tools for Bazel
Apache License 2.0
45 stars 30 forks source link

Don't include dependency management specifications as deps #44

Closed kchodorow closed 7 years ago

kchodorow commented 7 years ago

My previous implementation found the transitive dependencies of any dependency mentioned in the DependencyManagement section and added them all as dependencies. This was very incorrect and very bloated.

Now that the generate_workspace.bzl has been de-bloated, I've switched the BUILD files to using the generated java_libraries and ran unused_deps on everything.

Fixes #41.

kchodorow commented 7 years ago

AFAICT everything passed ("Executed 7 out of 7 tests: 7 tests pass.") so I'm not sure why the pipeline says it fails... summoning @damienmg.

petroseskinder commented 7 years ago

Jenkins says:

groovy.lang.MissingMethodException: No signature of method: build.bazel.ci.BazelUtils.copyCommands() is applicable for argument types: (java.util.ArrayList, java.util.HashMap) values: [[], [name:test.xml, uri:file:///home/ci/.cache/bazel/_bazel_ci/a123b34acc5de28efdcbead8be7ebf66/execroot/__main__/bazel-out/local-fastbuild/testlogs/generate_workspace/src/test/java/com/google/devtools/build/workspace/output/WorkspaceWriterTest/test.xml]]

Did the WorkspaceWriterTests also pass?

kchodorow commented 7 years ago

It looks it (//generate_workspace/src/test/java/com/google/devtools/build/workspace/output:WorkspaceWriterTest PASSED in 1.0s), so I'm guess a pipeline misconfiguration.

damienmg commented 7 years ago

Sorry I pushed a wrong config, just retrigger the build and it should be alright

On Tue, Jul 18, 2017, 8:05 PM Kristina notifications@github.com wrote:

It looks it (//generate_workspace/src/test/java/com/google/devtools/build/workspace/output:WorkspaceWriterTest PASSED in 1.0s), so I'm guess a pipeline misconfiguration.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/migration-tooling/pull/44#issuecomment-316147278, or mute the thread https://github.com/notifications/unsubscribe-auth/ADjHf_EFWl_YRod92HGV5qPZjpPcIo9Zks5sPPPVgaJpZM4ObrAI .

kchodorow commented 7 years ago

Cool, np. Test this please!

damienmg commented 7 years ago

BTW this looks like the PR I was waiting for, is it?

kchodorow commented 7 years ago

I have no idea, what were you waiting for?

damienmg commented 7 years ago

I had that dependencies explosion trying to pull test deps for testing Jenkins pipeline (the kind of test that would have prevented the error above :p)

On Tue, Jul 18, 2017, 8:18 PM Kristina notifications@github.com wrote:

I have no idea, what were you waiting for?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/migration-tooling/pull/44#issuecomment-316151211, or mute the thread https://github.com/notifications/unsubscribe-auth/ADjHf-3nWeas_MsngXqbHy6_wGd2iP9Oks5sPPcRgaJpZM4ObrAI .

kchodorow commented 7 years ago

Oh yeah! Well, here's the fix.

petroseskinder commented 7 years ago

LGTM. Tried it out with com.google.guava:guava:22.0 and it checks out. No unnecessary dependencies.

petroseskinder commented 7 years ago

@buchgr any status on the review

kchodorow commented 7 years ago

Given I've gotten an LGTM from @petroseskinder and it's been a couple days, going to submit.