Change in bazel[master]: Add initial thrift support.

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new change for review.

https://bazel-review.googlesource.com/2292

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl
9 files changed, 498 insertions(+), 0 deletions(-)



diff --git a/examples/thrift/AClient.java b/examples/thrift/AClient.java
new file mode 100644
index 0000000..2975e1f
--- /dev/null
+++ b/examples/thrift/AClient.java
@@ -0,0 +1,36 @@
+package io.bazel.examples.thrift;
+
+import java.lang.Integer;
+
+import org.apache.thrift.TException;
+import org.apache.thrift.protocol.TBinaryProtocol;
+import org.apache.thrift.protocol.TProtocol;
+import org.apache.thrift.transport.TSocket;
+import org.apache.thrift.transport.TTransport;
+
+public class AClient {
+ public static void main(String [] args) {
+ try {
+ Integer number = new Integer(args[0]);
+ TTransport transport;
+
+ transport = new TSocket("localhost", 9090);
+ transport.open();
+
+ TProtocol protocol = new TBinaryProtocol(transport);
+ AService.Client client = new AService.Client(protocol);
+
+ perform(client, number.intValue());
+
+ transport.close();
+ } catch (TException x) {
+ x.printStackTrace();
+ }
+ }
+
+ private static void perform(AService.Client client, int in_number)
throws TException
+ {
+ int out_number = client.GetInt(in_number);
+ System.out.println(in_number + "=" + out_number);
+ }
+}
diff --git a/examples/thrift/AServer.java b/examples/thrift/AServer.java
new file mode 100644
index 0000000..631b29b
--- /dev/null
+++ b/examples/thrift/AServer.java
@@ -0,0 +1,56 @@
+package io.bazel.examples.thrift;
+
+import java.net.InetSocketAddress;
+
+import org.apache.thrift.server.TServer;
+import org.apache.thrift.server.TServer.Args;
+import org.apache.thrift.server.TSimpleServer;
+import org.apache.thrift.transport.TServerSocket;
+import org.apache.thrift.transport.TServerTransport;
+import org.apache.thrift.TException;
+
+public class AServer {
+
+ static public class AServiceHandler implements AService.Iface {
+ @Override
+ public int GetInt(int n1){
+ System.out.println("n1: " + n1);
+ return n1;
+ }
+ }
+
+ public static AServiceHandler handler;
+
+ public static AService.Processor processor;
+
+ public static void main(String [] args) {
+ try {
+ handler = new AServer.AServiceHandler();
+ processor = new AService.Processor(handler);
+
+ Runnable simple = new Runnable() {
+ public void run() {
+ simple(processor);
+ }
+ };
+
+ new Thread(simple).start();
+ } catch (Exception x) {
+ x.printStackTrace();
+ }
+ }
+
+ public static void simple(AService.Processor processor) {
+ try {
+ InetSocketAddress address = new InetSocketAddress("127.0.0.1", 9090);
+ TServerTransport serverTransport = new TServerSocket(address);
+ TServer server = new TSimpleServer(new
Args(serverTransport).processor(processor));
+
+ System.out.println("Starting the simple server...");
+ server.serve();
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ }
+
+}
diff --git a/examples/thrift/BUILD b/examples/thrift/BUILD
new file mode 100644
index 0000000..04b4dad
--- /dev/null
+++ b/examples/thrift/BUILD
@@ -0,0 +1,77 @@
+load("/tools/build_rules/thrift/thrift",
+ "thrift_library",
+ "thrift_java_library")
+
+thrift_library(
+ name="thrift-liba",
+ srcs=["a.thrift"],
+ strip_prefix=".",
+)
+
+# should work
+thrift_java_library(
+ name="java-liba",
+ thrift_library=":thrift-liba",
+)
+
+thrift_library(
+ name="thrift-libb",
+ srcs=["b.thrift"],
+ deps=[":thrift-liba"],
+ strip_prefix=".",
+)
+
+# should work
+thrift_java_library(
+ name="java-libab",
+ thrift_library=":thrift-libb",
+)
+
+thrift_library(
+ name="thrift-liba2",
+ srcs=["a.thrift"],
+)
+
+thrift_library(
+ name="thrift-libb2",
+ srcs=["b.thrift"],
+ deps=[":thrift-liba2"],
+)
+
+# shouldn't work
+thrift_java_library(
+ name="java-libab2",
+ thrift_library=":thrift-libb2",
+)
+
+thrift_library(
+ name="thrift-libc",
+ srcs=["c.thrift"],
+ deps=[":thrift-liba2"],
+)
+
+# should work
+thrift_java_library(
+ name="java-libc",
+ thrift_library=":thrift-libc",
+)
+
+java_binary(
+ name="a-server",
+ srcs=[":AServer.java"],
+ deps=["//examples/thrift:java-liba",
+ "@org.apache.thrift_libthrift//jar",
+ "@org.slf4j_slf4j-api//jar",
+ ],
+ main_class="io.bazel.examples.thrift.AServer",
+)
+
+java_binary(
+ name="a-client",
+ srcs=[":AClient.java"],
+ deps=["//examples/thrift:java-liba",
+ "@org.apache.thrift_libthrift//jar",
+ "@org.slf4j_slf4j-api//jar",
+ ],
+ main_class="io.bazel.examples.thrift.AClient",
+)
diff --git a/examples/thrift/a.thrift b/examples/thrift/a.thrift
new file mode 100644
index 0000000..4602b26
--- /dev/null
+++ b/examples/thrift/a.thrift
@@ -0,0 +1,7 @@
+namespace java io.bazel.examples.thrift
+
+typedef i32 int
+
+service AService {
+ int GetInt(1: int n1);
+}
diff --git a/examples/thrift/b.thrift b/examples/thrift/b.thrift
new file mode 100644
index 0000000..4e5dcd7
--- /dev/null
+++ b/examples/thrift/b.thrift
@@ -0,0 +1,7 @@
+namespace java io.bazel.examples.thrift
+
+include "a.thrift"
+
+service BService {
+ a.int RPC1(1: a.int n1);
+}
diff --git a/examples/thrift/c.thrift b/examples/thrift/c.thrift
new file mode 100644
index 0000000..301af8c
--- /dev/null
+++ b/examples/thrift/c.thrift
@@ -0,0 +1,7 @@
+namespace java io.bazel.examples.thrift
+
+include "examples/thrift/a.thrift"
+
+service BService {
+ a.int RPC1(1: a.int n1);
+}
diff --git a/tools/build_rules/thrift/README.md
b/tools/build_rules/thrift/README.md
new file mode 100644
index 0000000..41f732d
--- /dev/null
+++ b/tools/build_rules/thrift/README.md
@@ -0,0 +1,114 @@
+# Thrift Rules
+
+<div class="toc">
+ <h2>Rules</h2>
+ <ul>
+ <li><a href="#thrift_library">thrift_library</a></li>
+ <li><a href="#thrift_java_library">thrift_java_library</a></li>
+ </ul>
+</div>
+
+## Overview
+
+These build rules are used for building [Thrift][thrift] files in Bazel.
+
+[thrift]: https://thrift.apache.org/
+
+<a name="setup"></a>
+## Setup
+
+To use the Thrift rules, simply copy the contents of the thrift.WORKSPACE
to
+your WORKSPACE file.
+
+<a name="thrift_library"></a>
+## thrift\_library
+
+```python
+thrift_library(name, srcs, deps, thriftopts)
+```
+<table>
+ <colgroup>
+ <col class="col-param" />
+ <col class="param-description" />
+ </colgroup>
+ <thead>
+ <tr>
+ <th>colspan="2">Attributes</th>
+ </tr>
+ </thead>
+ <tbody>
+ <tr>
+ <td><code>name</code></td>
+ <td>
+ <code>Name, required</code>
+ <p>A unique name for this rule.</p>
+ </td>
+ </tr>
+ <tr>
+ <td><code>srcs</code></td>
+ <td>
+ <code>List of labels, required</code>
+ <p>The thrift files that make up this library.</p>
+ </td>
+ <td><code>deps</code></td>
+ <td>
+ <code>List of labels, optional</code>
+ <p>List of Thrift libs depended on by the library.</p>
+ </td>
+ <td><code>thriftops</code></td>
+ <td>
+ <code>List of strings, optional</code>
+ <p>List of Thrift compile options. It is set to ["-strict"] by
+ default.</p>
+ </td>
+ <td><code>strip_prefix</code></td>
+ <td>
+ <code>String, optional<code>
+ <p>A prefix relative to package directory to strip from thrift
files'
+ paths packed into this library. The thrift lib will be relative to
the
+ workspace root if this parameter is not specified.</p>
+ </td>
+ </tr>
+ </tbody>
+</table>
+
+<a name="thrift_java_library"></a>
+## thrift\_java\_library
+
+```python
+thrift_java_library(name, thrift_library, deps, visibility)
+```
+<table>
+ <colgroup>
+ <col class="col-param" />
+ <col class="param-description" />
+ </colgroup>
+ <thead>
+ <tr>
+ <th>colspan="2">Attributes</th>
+ </tr>
+ </thead>
+ <tbody>
+ <tr>
+ <td><code>name</code></td>
+ <td>
+ <code>Name, required</code>
+ <p>A unique name for this rule.</p>
+ </td>
+ </tr>
+ <tr>
+ <td><code>thrift_library</code></td>
+ <td>
+ <code>Label, required</code>
+ <p>The thrift library to build into a java library.</p>
+ </td>
+ </tr>
+ <tr>
+ <td><code>deps</code></td>
+ <td>
+ <code>List of labels, optional</code>
+ <p>The list of java libraries that this library depends on.</p>
+ </td>
+ </tr>
+ </tbody>
+</table>
diff --git a/tools/build_rules/thrift/thrift.WORKSPACE
b/tools/build_rules/thrift/thrift.WORKSPACE
new file mode 100644
index 0000000..be4af9f
--- /dev/null
+++ b/tools/build_rules/thrift/thrift.WORKSPACE
@@ -0,0 +1,9 @@
+maven_jar(
+ name="org.apache.thrift_libthrift",
+ artifact="org.apache.thrift:libthrift:0.9.3"
+)
+
+maven_jar(
+ name="org.slf4j_slf4j-api",
+ artifact="org.slf4j:slf4j-api:1.7.12"
+)
diff --git a/tools/build_rules/thrift/thrift.bzl
b/tools/build_rules/thrift/thrift.bzl
new file mode 100644
index 0000000..0a5612b
--- /dev/null
+++ b/tools/build_rules/thrift/thrift.bzl
@@ -0,0 +1,185 @@
+# Copyright 2014 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+_thrift_filetype = FileType([".thrift"])
+_thriftlib_tar_filetype = FileType([".thriftlib.tar"])
+
+
+def _get_target_genfiles_root(ctx):
+ return ctx.label.package + ":" + ctx.label.name
+
+
+def _mkdir_command_string(path):
+ return "mkdir -p \"" + path + "\""
+
+
+def _cp_command_string(from_path, to_path):
+ return ("cp \"" + from_path + "\" " +
+ "\"" + to_path + "\"/")
+
+
+def _fix_timestamps_command_string(root):
+ return ("find \"" + root + "\" -mindepth 1" +
+ " -exec touch -t 198001010000 \"{}\" +")
+
+
+def _tar_extract_command_string(filename, to_path):
+ return "tar -xC \"" + to_path + "\"" + " -f \"" + filename + "\""
+
+
+def _thrift_java_compile_command_string(
+ include_dir, java_root, src, thrift_options):
+ return ("thrift " + " ".join(thrift_options)+ " -gen java" +
+ " -out \"" + java_root + "\"" +
+ " -I \"" + include_dir + "\" \"" + src.path + "\"")
+
+
+def _get_full_strip_prefix(ctx):
+ strip_prefix = ctx.attr.strip_prefix
+ if strip_prefix != "":
+ strip_prefix_label = ctx.label.relative(strip_prefix)
+ if strip_prefix == ".":
+ strip_prefix = ctx.label.package
+ else:
+ strip_prefix = "/".join([ctx.label.package,
+ strip_prefix])
+ for src in ctx.files.srcs:
+ if not src.path.startswith(strip_prefix):
+ fail("All srcs must be under strip_prefix path")
+ elif (len(src.path) != len(strip_prefix) and
+ src.path[len(strip_prefix)] != "/"):
+ fail("The strip_prefix parameter must be a directory")
+
+ return strip_prefix
+
+
+def _thrift_library_impl(ctx):
+ target_genfiles_root = _get_target_genfiles_root(ctx)
+
+ transitive_archive_files = set(order="compile")
+ for dep in ctx.attr.deps:
+ transitive_archive_files = transitive_archive_files.union(
+ dep._transitive_archive_files)
+ transitive_archive_files =
transitive_archive_files.union(dep.files)
+
+ commands = [
+ _mkdir_command_string(target_genfiles_root),
+ ]
+
+ strip_prefix = _get_full_strip_prefix(ctx)
+ for f in ctx.files.srcs:
+ file_dirname = f.dirname[len(strip_prefix):]
+ target_path = (target_genfiles_root + "/" + file_dirname)
+ commands.append(_mkdir_command_string(target_path))
+ commands.append(_cp_command_string(f.path, target_path))
+ commands.extend([
+ _fix_timestamps_command_string(target_genfiles_root),
+ "tar -cf \"" + ctx.outputs.libarchive.path + "\"" +
+ " -C \"" + target_genfiles_root + "\"" +
+ " .",
+ ])
+ ctx.action(
+ inputs = ctx.files.srcs,
+ outputs = [ctx.outputs.libarchive],
+ command = " && ".join(commands),
+ )
+ return struct(
+ srcs=ctx.files.srcs,
+ thriftopts=ctx.attr.thriftopts,
+ _transitive_archive_files=transitive_archive_files,
+ )
+
+
+thrift_library = rule(
+ _thrift_library_impl,
+ attrs={
+ "srcs": attr.label_list(allow_files=_thrift_filetype),
+ "deps": attr.label_list(allow_files=_thriftlib_tar_filetype),
+ "thriftopts": attr.string_list(default=["-strict"]),
+ "strip_prefix": attr.string(),
+ "_transitive_archive_files": attr.label_list(),
+ },
+ outputs={"libarchive": "lib%{name}.thriftlib.tar"},
+)
+
+
+def _gen_thrift_srcjar_impl(ctx):
+ target_genfiles_root = _get_target_genfiles_root(ctx)
+ thrift_includes_root = "/".join(
+ [ target_genfiles_root, "thrift_includes"])
+ gen_java_dir = "/".join([target_genfiles_root, "gen-java"])
+
+ commands = []
+ commands.append(_mkdir_command_string(target_genfiles_root))
+ commands.append(_mkdir_command_string(thrift_includes_root))
+
+ thrift_lib_archive_files =
ctx.attr.thrift_library._transitive_archive_files
+ for f in thrift_lib_archive_files:
+ commands.append(
+ _tar_extract_command_string(f.path, thrift_includes_root))
+
+ commands.append(_mkdir_command_string(gen_java_dir))
+ thrift_lib_srcs = ctx.attr.thrift_library.srcs
+ thrift_options = ctx.attr.thrift_library.thriftopts
+ for src in thrift_lib_srcs:
+ commands.append(_thrift_java_compile_command_string(
+ thrift_includes_root, gen_java_dir, src, thrift_options))
+ commands.append(_fix_timestamps_command_string(gen_java_dir))
+
+ out = ctx.outputs.srcjar
+ commands.append(ctx.file._jar.path + " cMf \"" + out.path + "\"" +
+ " -C \"" + gen_java_dir + "\" .")
+
+ inputs = (
+ list(thrift_lib_archive_files) + thrift_lib_srcs + [ctx.file._jar]
+ ctx.files._jdk)
+
+ ctx.action(
+ inputs = inputs,
+ outputs = [ctx.outputs.srcjar],
+ command = " && ".join(commands),
+ )
+
+
+thrift_java_srcjar = rule(
+ _gen_thrift_srcjar_impl,
+ attrs={
+ "thrift_library": attr.label(
+ mandatory=True,
providers=['srcs', '_transitive_archive_files']),
+ "_jar": attr.label(
+ default=Label("@bazel_tools//tools/jdk:jar"),
+ allow_files=True,
+ single_file=True),
+ "_jdk": attr.label(
+ default=Label("@bazel_tools//tools/jdk:jdk"),
+ allow_files=True),
+ },
+ outputs={"srcjar": "lib%{name}.srcjar"},
+)
+
+
+def thrift_java_library(name, thrift_library, deps=[], visibility=None):
+ thrift_java_srcjar(
+ name=name + '_srcjar',
+ thrift_library=thrift_library,
+ visibility=visibility,
+ )
+ native.java_library(
+ name=name,
+ srcs=[name + '_srcjar'],
+ deps=deps + [
+ "@org.apache.thrift_libthrift//jar",
+ "@org.slf4j_slf4j-api//jar",
+ ],
+ visibility=visibility,
+ )

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#2).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

9 files changed, 487 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 2

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#3).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

9 files changed, 487 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 3

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#4).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

9 files changed, 487 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 4

[Warren Turkal (Gerrit)]

Warren Turkal has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 4:

(1 comment)

I've tried to follow the style of other files in the repo for the different
language files. I also had a question about the structure of my BUILD
example.

Thanks for looking! wt

https://bazel-review.googlesource.com/#/c/2292/4/examples/thrift/BUILD
File examples/thrift/BUILD:

PS4, Line 30: thrift_library(
: name="thrift-liba2",
: srcs=["a.thrift"],
: )
:
: thrift_library(
: name="thrift-libb2",
: srcs=["b.thrift"],
: deps=[":thrift-liba2"],
: )
:
: # shouldn't work
: thrift_java_library(
: name="java-libab2",
: thrift_library=":thrift-libb2",
: )
These rules make sure that omitting the strip_prefix in the "thrift-liba2"
target cause the java lib (actually a srcjar) to be unable to build. Is
this something that I should keep in this example?

Also, is there a better way to do build tests that are expected to fail
like this?

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 4

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>
Gerrit-HasComments: Yes

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#5).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

9 files changed, 487 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 5

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#6).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---

M WORKSPACE

A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

10 files changed, 495 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 6

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#7).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

9 files changed, 485 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 7

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

[David Chen (Gerrit)]

David Chen has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 7:

(3 comments)

https://bazel-review.googlesource.com/#/c/2292/7/examples/thrift/BUILD
File examples/thrift/BUILD:

Line 61: srcs=[":AServer.java"],
You don't need the ':' at the beginning here. Same with AClient.java


https://bazel-review.googlesource.com/#/c/2292/7/tools/build_rules/thrift/thrift.bzl
File tools/build_rules/thrift/thrift.bzl:

Line 96: thrift_library = rule(
Might be better to move all rule declarations together to the bottom.


Line 144: thrift_java_srcjar = rule(
Is this meant to be a public rule? If not, prepend the name with '_' to
make it a private symbol.

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 7

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>

[David Chen (Gerrit)]

David Chen has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 7:

Looks good overall. Thanks for implementing this!

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 7

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: No

[David Chen (Gerrit)]

David Chen has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 4:

(1 comment)

https://bazel-review.googlesource.com/#/c/2292/4/examples/thrift/BUILD
File examples/thrift/BUILD:

PS4, Line 30: thrift_library(
: name="thrift-liba2",
: srcs=["a.thrift"],
: )
:
: thrift_library(
: name="thrift-libb2",
: srcs=["b.thrift"],
: deps=[":thrift-liba2"],
: )
:
: # shouldn't work
: thrift_java_library(
: name="java-libab2",
: thrift_library=":thrift-libb2",
: )
> These rules make sure that omitting the strip_prefix in the "thrift-liba2"

If you want to add rule tests, especially negative tests, you can also try
using the Skylark testing framework:
https://github.com/bazelbuild/bazel/blob/master/tools/build_rules/test_rules.bzl

There are some examples of the rule tests for the following rule sets:

* Rust:
https://github.com/bazelbuild/bazel/tree/master/tools/build_rules/rust/test
* Sass:
https://github.com/bazelbuild/bazel/tree/master/tools/build_defs/sass/test

The caveat is that the testing rules are still experimental and don't have
much documentation yet. tunes@ - are we ready to add documentation for the
Skylark test rules yet?

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 4

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: Yes

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#8).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md

A tools/build_rules/thrift/test/BUILD
A tools/build_rules/thrift/test/thrift_rule_test.bzl
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl
11 files changed, 540 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 8

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#9).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---

M .gitignore
M WORKSPACE

A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md

A tools/build_rules/thrift/test/BUILD
A tools/build_rules/thrift/test/thrift_rule_test.bzl
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

13 files changed, 551 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 9

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#10).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java
A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md

A tools/build_rules/thrift/test/BUILD
A tools/build_rules/thrift/test/thrift_rule_test.bzl
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

11 files changed, 540 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 10

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#11).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java

A examples/thrift/BClient.java
A examples/thrift/BServer.java

A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md

A tools/build_rules/thrift/test/BUILD
A tools/build_rules/thrift/test/thrift_rule_test.bzl
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

13 files changed, 658 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 11

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 11:

(4 comments)

Hey there,

I think I have addressed all the comments. I have done the following in
this patchset:

* added testing for skylark rules
* changed a couple of absolute references to relative references in one of
the BUILD files
* made some of the thrift files a little more complex to make sure I am
using examples that actually use their dependencies

With regard to the testing of the skylark rules, all the rules I added are
positive tests. I don't see any examples of, nor is it obvious to me how to
construct, negative tests for rules that I expect to fail, like
//examples/thrift:java-libab2. Do you have any suggestions?

I also see one other thing that I was wondering if you had any insight
about. I don't like the fact that I essentially have to keep two lists of
the same deps at times. Looks at this example:

thrift_library(
name="thrift-libb",
srcs=["b.thrift"],
deps=[":thrift-liba"],
strip_prefix=".",
)

thrift_java_library(
name="java-libab",
thrift_library=":thrift-libb",
deps=[":java-liba"],
)

Notice that I have to include "thrift-liba" as a dep of the thrift lib
and "java-liba" as a dep of the java lib. Is there any way to do something
smarter so that I don't have to maintain these parallel deps for each level
of the deps. Ideally, I would like to only have to maintain the dep in the
thrift_library target. I am happy to expand thrift_java_library into one or
more rules if there is a way to make this happen. Do you have any
suggestions for this problem?

wt

https://bazel-review.googlesource.com/#/c/2292/4/examples/thrift/BUILD
File examples/thrift/BUILD:

PS4, Line 30: deps=[":java-liba"],
: )
:

: thrift_library(
: name="thrift-liba2",
: srcs=["a.thrift"],
: )
:
: thrift_library(
: name="thrift-libb2",
: srcs=["b.thrift"],
: deps=[":thrift-liba2"],
: )
:
: # shouldn't work
: t

> If you want to add rule tests, especially negative tests, you can also try

I've added a number of positive tests. However, I am not seeing any
examples of negative tests, so I am not sure how to write a test for a rule
that I expect to fail. Do you have any pointers?

https://bazel-review.googlesource.com/#/c/2292/7/examples/thrift/BUILD
File examples/thrift/BUILD:

Line 61:

> You don't need the ':' at the beginning here. Same with AClient.java

Done


https://bazel-review.googlesource.com/#/c/2292/7/tools/build_rules/thrift/thrift.bzl
File tools/build_rules/thrift/thrift.bzl:

Line 96: thrift_includes_root = "/".join(

> Might be better to move all rule declarations together to the bottom.

Done


Line 144: attrs={

> Is this meant to be a public rule? If not, prepend the name with '_' to
> mak

Done

However, is there any way to a expose these private symbols for testing?
Testing only the public rules leaves a lot of room for error. I'd love to
be able to test narrower bits.

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 11

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: Yes

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#12).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java

A examples/thrift/BClient.java
A examples/thrift/BServer.java

A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md

A tools/build_rules/thrift/test/BUILD
A tools/build_rules/thrift/test/thrift_rule_test.bzl
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

13 files changed, 654 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 12

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 12:

Also, should I move from using tar to using jar for the archiving of the
thrift libraries? It seems that I can use jar as a generic archiving
utility. If I do that, I wouldn't need to introduce a new dep on tar.

wt

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 12

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: No

[Warren Turkal (Gerrit)]

Warren Turkal has uploaded a new patch set (#13).

Change subject: Add initial thrift support.
......................................................................

Add initial thrift support.

This support includes the following a couple rules and a macro for
building the thrift files into java source. It also includes an example
java client and server and documentation for the rules.

Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Signed-off-by: Warren Turkal <w...@penguintechs.org>
---
A examples/thrift/AClient.java
A examples/thrift/AServer.java

A examples/thrift/BClient.java
A examples/thrift/BServer.java

A examples/thrift/BUILD
A examples/thrift/a.thrift
A examples/thrift/b.thrift
A examples/thrift/c.thrift
A tools/build_rules/thrift/README.md

A tools/build_rules/thrift/test/BUILD
A tools/build_rules/thrift/test/thrift_rule_test.bzl
A tools/build_rules/thrift/thrift.WORKSPACE
A tools/build_rules/thrift/thrift.bzl

13 files changed, 667 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Warren Turkal (Gerrit)]

Warren Turkal has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 13:

Is there any way to tap into the logic of pkg_tar to simplify the logic of
the thrift_library target?

wt

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: No

[David Chen (Gerrit)]

David Chen has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 13:

Hi Warren, sorry for the delay. I've been oncall for the past week or so
and have been a bit busy.

I recently saw that the protobuf team wrote their own Skylark protobuf
rules for C++ and Python:
https://github.com/google/protobuf/blob/master/protobuf.bzl. Maybe you can
do something similar to the cc_proto_library?

dslomov@ - Do you have any thoughts about this?

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Dmitry Lomov <dsl...@google.com>

[David Chen (Gerrit)]

David Chen has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 13:

> Also, should I move from using tar to using jar for the archiving
> of the thrift libraries? It seems that I can use jar as a generic
> archiving utility. If I do that, I wouldn't need to introduce a new
> dep on tar.
>
> wt

That sounds fine for Java Thrift libraries, but I don't think it would be a
good idea to use jar if we want to extend this to support other languages
as well.

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Damien Martin-guillerez (Gerrit)]

Damien Martin-guillerez has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 13:

friendly ping

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: Damien Martin-guillerez <dmar...@google.com>

[David Chen (Gerrit)]

David Chen has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 13:

dslomov@ - Are Skylark aspects ready for use yet? Perhaps we can use this
as an early customer of Skylark aspects?

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Dmitry Lomov (Gerrit)]

Dmitry Lomov has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Yes, sorry for late reply.
Skylark aspects are in and should be feature-complete

Yes, sorry for the late reply - I am just back from vacation.

Aspects are designed to help with exactly this dependency problem: with
aspects, you can augment existing dependency graph with missing
dependencies automatically.

Skylark aspects are in (very recently!), and they should be
feature-complete for this. I am still working on public documentation for
Skylark aspects - it you are serious about pursuing this, I can prioritize
that.

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

[Laurent Le Brun (Gerrit)]

Laurent Le Brun has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................

Patch Set 13: Code-Review+1

(1 comment)

No objection from my side

https://bazel-review.googlesource.com/#/c/2292/13/tools/build_rules/thrift/thrift.bzl
File tools/build_rules/thrift/thrift.bzl:

Line 19: return ctx.label.package + ":" + ctx.label.name
I think str(ctx.label) is equivalent.

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Dmitry Lomov <dsl...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: Yes

[Damien Martin-guillerez (Gerrit)]

Damien Martin-guillerez has posted comments on this change.

Change subject: Add initial thrift support.
......................................................................


Patch Set 13:

Is this code review still going on?

Note that we should do a new repository if we agree to merge it. I think we
should do a different process for new rules now.

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cc9058fe151bc81b2e99fdd1d45d1d800fad879
Gerrit-PatchSet: 13
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Warren Turkal <w...@penguintechs.org>

Gerrit-Reviewer: Damien Martin-guillerez <dmar...@google.com>

Gerrit-Reviewer: David Chen <d...@google.com>
Gerrit-Reviewer: Dmitry Lomov <dsl...@google.com>
Gerrit-Reviewer: Laurent Le Brun <laur...@google.com>
Gerrit-Reviewer: Warren Turkal <w...@penguintechs.org>

Gerrit-HasComments: No

[Damien Martin-guillerez (Gerrit)]

Damien Martin-guillerez has abandoned this change.

Change subject: Add initial thrift support.
......................................................................

Abandoned

--
To view, visit https://bazel-review.googlesource.com/2292
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: abandon