bazeltools / bazel-deps

Generate bazel dependencies for maven artifacts
MIT License
250 stars 121 forks source link

maven tool requires all nodes for a dependency #225

Open milesmatthias opened 5 years ago

milesmatthias commented 5 years ago

See my notes in #223 for more context.

Running the maven tool to parse a pom.xml and turn it into a dependencies.yaml will throw an error that you're missing a node if you don't have all of the nodes it's expecting for a dependency listing.

For example, I had this dependency:

        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-web</artifactId>
            <version>2.1.0.RELEASE</version>
            <classifier>spring-starter-web-classifier</classifier>
            <scope>spring-starter-web-scope</scope>
            <type>spring-starter-web-type</type>
        </dependency>

that gave me that error until I added the classifier, scope, and type nodes. The tool didn't check the values, but I need to have a value in there.

Those fake values were written to my dependencies.yaml file, but I simply removed them so that running the next step (bazel run parse) would work as expected.

johnynek commented 5 years ago

oh man... sorry about this. We need to add tests for this maven migration tool so we don't have regressions like this.

milesmatthias commented 5 years ago

Thanks @johnynek, at least there's a workaround. I'm not a scala person, but if I get a chance I'll take a quick whack at it. The show stopper is really #224 though, so if you have any thoughts on a workaround there, that'd be much appreciated.

kwiesmueller commented 5 years ago

Is there a fix for this yet?

johnynek commented 5 years ago

I try to review and merge PRs with tests pretty quickly. Unfortunately I and my colleagues at Stripe tend to only address problems we are hitting directly and we only used this maven migration code a few times in the past and didn’t need it to be perfect: we manually fixed any issues later.

Sorry I don’t have time to work on this issue.

kwiesmueller commented 5 years ago

Okay, yeah that's what we did now as well and i guess we'll only run it a few times as well. Thanks for the update.