Use protobuf instead of dot to communicate between parsers and core #13

cgrushko · 2017-10-06T22:21:51Z

that is, between the language-specific parsers and the language-agnostic core.

petroseskinder

I asked for a few small changes. Mostly nits

That said, let me understand the context. You have moved entirely away from using jdeps for obtaining the class dependency graph? That was why we previously used the dot/graphviz format for representing the class graph.

petroseskinder · 2017-10-07T00:20:58Z

src/main/java/com/google/devtools/build/bfg/Bfg.java

@@ -108,6 +111,7 @@ private void run(String[] args) throws Exception {
    cmdLineParser.parseArgument(args);

    List<String> dotFile = readDotFileFromStdIn();


Do you need this (or line 223) anymore?
If so, in what context will both exist?

Sorry, didn't do a good job cleaning up. Removed.

petroseskinder · 2017-10-07T01:10:07Z

src/main/java/com/google/devtools/build/bfg/bfg.proto

+    // In the Java case, we have (u, v) if 'u' mentions [1] 'v'.
+    // [1] - By mention, we mean either imports or references in code, either as a fully qualified
+    //       name, or an implied same-package name.
+    map<string, Strings> class_to_class = 1;


can you rename to class_dependencies.
getClassDependencyMap() is much clearer than getClassToClassMap()

"dependencies" can mean several things, I think that "class to class" describes it more precisely.
There are soon going to be additional fields (class_to_file, file_to_rule_kind) which will be consistent with the new name.
What do you think?

There are soon going to be additional fields (class_to_file, file_to_rule_kind) which will be consistent with the new name.

This is the portion that is unclear to me, the future direction. Can you expand on when and how these additional fields would be used? How would you obtain these file_to_rule dependencies?

That aside, from my limited vantage point, these are all dependency graphs. The nodes, (i.e. type of dependency) may differ but at their core, they are dependencies. For example, an edge between a file and another rule indicates a dependency between that file and that rule. Consequently, I am curious why not define something like

message ParserOutput { map<Deps, Deps> dependency_map = 1; } message Dep { string name; string type; } message Deps { repeated Dep deps; }

Note, the main takeaway you should get out of my message is not the specific proto I defined. It's that I don't understand what the direction is.

I concede on this

petroseskinder · 2017-10-07T01:13:20Z

src/main/java/com/google/devtools/build/bfg/bfg.proto

+    map<string, Strings> class_to_class = 1;
+}
+
+message Strings {


like above, can you make this some variant of dependency like deps or what have you?

I'm going to reuse this for other graphs (class_to_file, file_to_rule_kind) so "dependency" doesn't fit.
OTOH defining extra messages just for the name sounds wasteful.

I am fine accepting class_to_class, but Strings (and s) is... sketchy. It's overly broad.

OTOH defining extra messages just for the name sounds wasteful.

Understandable complaint. However, using overly broad or single letter variable names in Java is inappropriate and non-idiomatic. Perhaps it's not an issue when looking at the proto in isolation, but it's hard to argue the below is proper java code

private static HashMap<String, Bfg.Strings> writeGraphToProtoMap; // you defined your own special Strings class. Why??? ... adj.addAllS(graph.adjacentNodes(u)); // what is S???

I'm going to reuse this for other graphs (class_to_file, file_to_rule_kind)

there has to be some unifying quality about each of those, beyond Strings. Perhaps node.

As a side note, if we move to scala, i will be l

How about repeated string elements instead of repeated string s?

petroseskinder · 2017-10-07T01:18:00Z

src/main/java/com/google/devtools/build/bfg/BUILD

+java_proto_library( 
+    name = "bfg_java_proto"
+, deps = [":bfg_proto" ], 
+)


nit: new line

petroseskinder · 2017-10-07T01:25:39Z

src/main/java/com/google/devtools/build/bfg/Bfg.java

@@ -108,6 +111,7 @@ private void run(String[] args) throws Exception {
    cmdLineParser.parseArgument(args);

    List<String> dotFile = readDotFileFromStdIn();
+    ParserOutput parserOutput = ParserOutput.parseFrom(System.in);
    if (dotFile.isEmpty()) {


remove?

what arguments are you testing this against?
From what I understand of the change, there will be no more dotfiles, and also no more jdeps. Consequently, this should result in error.

petroseskinder · 2017-10-07T01:32:48Z

src/test/java/com/google/devtools/build/bfg/BUILD

@@ -1,18 +1,6 @@
 licenses(["notice"])  # Apache 2.0

 java_test(
-    name = "DotFileParserTest",


is it customary to test protobuffs?

do you mean if's customary to test the proto parser?
the parser has tons of tests in its own codebase, so no.
That's one big reason to use other's well tested code, you don't need to test it yourself :)

do you mean if's customary to test the proto parser?

Somewhat. What I was more concerned about was how we were unable to detect that dotfile.isEmpty() was not removed. Does bfg currently have e2e tests?

That's one big reason to use other's well tested code, you don't need to test it yourself :)

SGTM

Agreed, there are no e2e tests which would've caught it.
My approach is to hold off e2e tests as long as possible because they're really annoying to set up, at least inside Google. I think it's still fine to hold off, because there's little complexity in Bfg.java.

My approach is to hold off e2e tests as long as possible because they're really annoying to set up, at least inside Google. I think it's still fine to hold off, because there's little complexity in Bfg.java.

SGTM

that is, between the language-specific parsers and the language-agnostic core.

cgrushko · 2017-10-07T14:12:05Z

retest this please

…solved

petroseskinder

@cgrushko you're free to merge. :)
There was one nit that annoyed me. Can you make sure to change that.

petroseskinder · 2017-10-07T17:57:26Z

src/main/java/com/google/devtools/build/bfg/bfg.proto

+    map<string, Strings> class_to_class = 1;
+}
+
+message Strings {


I am fine accepting class_to_class, but Strings (and s) is... sketchy. It's overly broad.

OTOH defining extra messages just for the name sounds wasteful.

Understandable complaint. However, using overly broad or single letter variable names in Java is inappropriate and non-idiomatic. Perhaps it's not an issue when looking at the proto in isolation, but it's hard to argue the below is proper java code

private static HashMap<String, Bfg.Strings> writeGraphToProtoMap; // you defined your own special Strings class. Why??? ... adj.addAllS(graph.adjacentNodes(u)); // what is S???

I'm going to reuse this for other graphs (class_to_file, file_to_rule_kind)

there has to be some unifying quality about each of those, beyond Strings. Perhaps node.

As a side note, if we move to scala, i will be l

petroseskinder · 2017-10-07T17:57:35Z

src/main/java/com/google/devtools/build/bfg/bfg.proto

+    // In the Java case, we have (u, v) if 'u' mentions [1] 'v'.
+    // [1] - By mention, we mean either imports or references in code, either as a fully qualified
+    //       name, or an implied same-package name.
+    map<string, Strings> class_to_class = 1;


I concede on this

petroseskinder · 2017-10-07T18:01:13Z

test this please

… is resolved" This reverts commit 8ff7abe.

…azelbuild#13) * Use protobuf instead of dot to communicate between parsers and core that is, between the language-specific parsers and the language-agnostic core. * Move .bazelrc to tools/ so ci.bazel.build will not override it * Remove our .bazelrc until bazelbuild/continuous-integration#141 is resolved * Revert "Remove our .bazelrc until bazelbuild/continuous-integration#141 is resolved" This reverts commit 8ff7abe. * Rename tools/.bazelrc to tools/bazel.rc * Rename proto field to 'elements'

cgrushko · 2017-10-11T19:36:31Z

Re commit message - I think it would be better to only use the initial commit message (e.g., without 'Revert "Remove out"...').
Unless things have considerably changed in the review, the commit message that's part of the PR should usually do.

petroseskinder · 2017-10-11T19:40:19Z

Unless things have considerably changed in the review, the commit message that's part of the PR should usually do.

SGTM

cgrushko requested a review from petroseskinder October 6, 2017 22:21

cgrushko mentioned this pull request Oct 6, 2017

Respect user-defined .bazelrc bazelbuild/continuous-integration#141

Closed

petroseskinder suggested changes Oct 7, 2017

View reviewed changes

Use protobuf instead of dot to communicate between parsers and core

f096a9f

that is, between the language-specific parsers and the language-agnostic core.

cgrushko force-pushed the proto branch from ed349e2 to f096a9f Compare October 7, 2017 14:05

Move .bazelrc to tools/ so ci.bazel.build will not override it

846b686

Remove our .bazelrc until bazelbuild/continuous-integration#141 is re…

8ff7abe

…solved

petroseskinder mentioned this pull request Oct 7, 2017

Add e2e tests #14

Open

petroseskinder approved these changes Oct 7, 2017

View reviewed changes

cgrushko added 3 commits October 7, 2017 16:18

Revert "Remove our .bazelrc until bazelbuild/continuous-integration#141…

002f4d0

… is resolved" This reverts commit 8ff7abe.

Rename tools/.bazelrc to tools/bazel.rc

d85cfcc

Rename proto field to 'elements'

fce8200

petroseskinder merged commit 1180218 into bazelbuild:master Oct 8, 2017

cgrushko deleted the proto branch October 8, 2017 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use protobuf instead of dot to communicate between parsers and core #13

Use protobuf instead of dot to communicate between parsers and core #13

cgrushko commented Oct 6, 2017

petroseskinder left a comment

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017

petroseskinder Oct 7, 2017

petroseskinder Oct 7, 2017

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017 •

edited

Loading

petroseskinder Oct 8, 2017

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017 •

edited

Loading

cgrushko Oct 7, 2017

petroseskinder Oct 7, 2017

cgrushko commented Oct 7, 2017

petroseskinder left a comment

petroseskinder Oct 7, 2017

petroseskinder Oct 7, 2017

petroseskinder commented Oct 7, 2017

cgrushko commented Oct 11, 2017

petroseskinder commented Oct 11, 2017

		@@ -108,6 +111,7 @@ private void run(String[] args) throws Exception {
		cmdLineParser.parseArgument(args);

		List<String> dotFile = readDotFileFromStdIn();

Use protobuf instead of dot to communicate between parsers and core #13

Use protobuf instead of dot to communicate between parsers and core #13

Conversation

cgrushko commented Oct 6, 2017

petroseskinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgrushko Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petroseskinder Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgrushko commented Oct 7, 2017

petroseskinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petroseskinder commented Oct 7, 2017

cgrushko commented Oct 11, 2017

petroseskinder commented Oct 11, 2017

cgrushko Oct 7, 2017 •

edited

Loading

petroseskinder Oct 7, 2017 •

edited

Loading