[WIP] Create and end-to-end test of the Java lang parser and BFG.

[greggdonovan]

Changes needed to make this pass:

• Returning Buildozer command fragments for certain targets in the language-specific parsers that cannot be deduced by the BFG. E.g.: a main class for a java_binary.
• Adding graph nodes for all targets found to ensure that BUILD files are generated for disconnected targets.
• Generalizing the heuristics for merging different rule kinds within the same strongly connected component.

TODOs

• Should the e2e script be an sh_test or implemented natively in Skylark? How will we want it to work if we expand this to tests that use Maven, Gradle, Sbt, etc.?
• Adding e2e tests that use external dependencies.

[cgrushko]

Whoa, this is awesome!

I feel it'll be easier to review if the "changes needed" were separate, though; I believe they're largely independent? Seems like they fix genuine bugs and missing features.

[greggdonovan]

I feel it'll be easier to review if the "changes needed" were separate, though; I believe they're largely independent?

@cgrushko That's a good idea! I started with the Bash script and the PR expanded under everything went green. I'll chop this up into smaller parts that will be easier to review.

[greggdonovan]

Closing this PR, as I will split it up into new smaller PRs.

[cgrushko]

Sounds good :) I have a first nit, I'm afraid - in my opinion RuleKind is ok to use for the new message, no need for TargetInfo. :|

[greggdonovan]

Sounds good :) I have a first nit, I'm afraid - in my opinion RuleKind is ok to use for the new message, no need for TargetInfo. :|

No worries -- nit away! :)

So, use RuleKind as the name of the proto message? E.g.:

message ParserOutput {
...
 map<string, RuleKind> file_to_rule_kind = 3;
}

message RuleKind {
    required string rule_kind = 1;
    optional Strings buildozer_commands = 2;
}
...

[cgrushko]

Yeah, I think that even if it's technically more than RuleKind, anyone glancing at the code will be able to deduce the intention.

Then again... mm... the java_binary example has a main_class, which is specific to that particular file, and is not a kind in general. So now I'm not so sure anymore.

Do we still need rule_kind if we have the buildozer_commands?

[greggdonovan]

Do we still need rule_kind if we have the buildozer_commands?

That's a good question -- I thought about that too. We could include it in the buildozer_commands and parse out the rule kind when we need it from set kind <rule_kind>. I included rule_kind as a separate field to make it clear, semantically, that it was required.

[cgrushko]

Ah, I'd rather not parse Buildozer commands. But why do we need set kind? I thought it'd be in the buildozer_commands so BFG just runs the commands. Or is it required to merge rule kinds?

[greggdonovan]

Or is it required to merge rule kinds?

Exactly. If we have, say, a java_library, java_binary, and java_image target in the same connected component we need to decide which one it should be.

We also need to decide to error out if we hit a un-mergeable condition. Say, a java_test and a java_binary in the same connected component. Or, worse, a py_binary and a java_library somehow ending up in the same connected component.

This was my attempt to come up with some reasonble cross-language heuristics for how and when we can and can't merge different kinds in a single component.

[cgrushko]

For the name, I still can't decide :)

For merging, perhaps the parser will specify a graph of kinds, library -> binary library -> test so that BFG can merge library with binary, but not test with binary. (in which case it'll error out)

Cross-language merging sounds like a big problem, because I had an assumption that parsers will be independent of each other. Does cross-language merging come up a lot?

[greggdonovan]

For merging, perhaps the parser will specify a graph of kinds, library -> binary library -> test so that BFG can merge library with binary, but not test with binary. (in which case it'll error out)

That's an interesting idea! Having language specific parsers emit the merge rules that they approve seems like a clearer division of responsibility than having the BFG do all of the heuristics.

Cross-language merging sounds like a big problem, because I had an assumption that parsers will be independent of each other. Does cross-language merging come up a lot?

The case I was thinking of was a set of Scala and Java files in the same connected component. Say:

src/main/java/{A,B}.java
src/main/scala/{C,D}.scala

where A.java depends on D.scala and C.scala depends onB.java.

Sbt will compile them all together despite the cyclic dependency between the two directories.

Hopefully these cases will be rare. And I don't think scala_library would even support it now.

[ittaiz]

Yes it does :) I added support for this a year and a half ago since it was a blocker for us :)

On Fri, Dec 1, 2017 at 8:50 PM Gregg Donovan notifications@github.com wrote:

For merging, perhaps the parser will specify a graph of kinds, library -> binary library -> test so that BFG can merge library with binary, but not test with binary. (in which case it'll error out)

That's an interesting idea! Having language specific parsers emit the merge rules that they approve seems like a clearer division of responsibility than having the BFG do all of the heuristics.

Cross-language merging sounds like a big problem, because I had an assumption that parsers will be independent of each other. Does cross-language merging come up a lot?

The case I was thinking of was a set of Scala and Java files in the same connected component. Say:

src/main/java/{A,B}.java src/main/scala/{C,D}.scala

where A.java depends on D.scala and C.scala depends onB.java.

Sbt will compile them all together despite the cyclic dependency between the two directories.

Hopefully these cases will be rare. And I don't think scala_library would even support it now.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/BUILD_file_generator/pull/32#issuecomment-348576862, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUIF9HA2euQv6URqKJpZDc7a5ETD_UVks5s8EpbgaJpZM4QrtBb .

[greggdonovan]

@ittaiz Fantastic -- thanks. :)