Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose proto fragment fields necessary for writing proto_toolchain to Starlark #9000

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,7 @@ java_library(
":util",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/analysis/skylark/annotations",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.rules.proto;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
Expand All @@ -23,6 +24,7 @@
import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.skylark.annotations.SkylarkConfigurationField;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skylarkbuildapi.ProtoConfigurationApi;
Expand Down Expand Up @@ -230,6 +232,7 @@ private ProtoConfiguration(Options options) {
this.options = options;
}

@Override
public ImmutableList<String> protocOpts() {
return protocOpts;
}
Expand All @@ -243,6 +246,20 @@ public boolean runExperimentalProtoExtraActions() {
return options.experimentalProtoExtraActions;
}

@SkylarkConfigurationField(
// Must match the value of `_protoc_key`
// in `@rules_proto//proto/private/rules:proto_toolchain.bzl`.
name = "protoc_do_not_use_or_we_will_break_you_without_mercy",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan for this command line option?

The usual thing we do is to have rules like CcToolchainAliasRule, although those can just as well be implemented in Starlark. That way, you don't need to have a late-bound default. Even better, you can prescribe a particular name for that rule so that people use it as little as possible (although we don't do that for the rest of the alias rules)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these options here are meant for use in a new proto_toolchain rule that is eventually going to replace the command-line options.

i.e. Now, proto_toolchain exposes the options and looks like this: https://github.com/bazelbuild/rules_proto/blob/919671ef0f73f5a82037eaabedc39c938c13cf60/proto/BUILD#L8, and eventually, it will look like this, and the command-line options are deprecated and removed:

proto_toolchain(
    name = "default_toolchain_impl",
    compiler = "@com_google_protobuf//:protoc",
    protocopt = ["--foo", "--bar"],
    strict_deps = "ERROR",
    visibility = ["//visibility:public"],
)

I guess we could also implement proto_toolchain as native rule first, but since we're going to replace it with a Starlark rule eventually, we can also implement it Starlark now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my question I already sent is answered here :) So you plan to replace command line options completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, long-term (>1y), I'd like to get rid of the (native) command-line options.

doc = "Exposes the value of `--proto_compiler`."
+ "<p><b>Do not use this field directly</b>, its only purpose is to help with "
+ "migration of Protobuf rules to Starlark.</p>"
+ "<p>Instead, you can access the value through proto_toolchain</p>"
+ "<p><pre class=\"language-python\">\n"
+ "def _my_rule_impl(ctx):"
+ " proto_toolchain = ctx.toolchains[\"@rules_proto//proto:toolchain\"]\n"
+ " protoc = proto_toolchain.compiler\n"
+ "</pre></p>"
)
public Label protoCompiler() {
return options.protoCompiler;
}
Expand All @@ -267,6 +284,23 @@ public StrictDepsMode strictProtoDeps() {
return options.strictProtoDeps;
}

@Override
public String starlarkStrictDeps() {
switch (strictProtoDeps()) {
case OFF: // fall-through
case DEFAULT:
return "OFF";

case WARN:
return "WARN";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually use "WARN"? AFAIU it's only used for the Java sandwich, but that's passed in from Starlark. And if not, how about using a Boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, WARN==ERROR (and DEFAULT==OFF [1], but the default value for --strict_proto_deps is error 🤷‍♀). I'd like proto_library to support WARN eventually, and also print some nice buildozer commands when there are errors or warnings, similar to what Java does.

[1] https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java;l=616?q=areDepsStrict


case ERROR: // fall-through
case STRICT:
return "ERROR";
}
throw new IllegalStateException();
}

public List<String> ccProtoLibraryHeaderSuffixes() {
return ccProtoLibraryHeaderSuffixes;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.devtools.build.lib.skylarkbuildapi;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
import com.google.devtools.build.lib.syntax.StarlarkValue;
Expand All @@ -22,5 +24,65 @@
@SkylarkModule(
name = "proto",
category = SkylarkModuleCategory.CONFIGURATION_FRAGMENT,
doc = "A configuration fragment representing protocol buffers.")
public interface ProtoConfigurationApi extends StarlarkValue {}
doc = "A configuration fragment representing protocol buffers. "
Yannic marked this conversation as resolved.
Show resolved Hide resolved
+ "<p><b>Do not use these fields directly<b>, they are considered an implementation "
+ "detail and will be removed after migrating Protobuf rules to Starlark.</p>"
// TODO(yannic): Link to generated docs of `proto_toolchain`.
// https://github.com/bazelbuild/bazel/issues/9203
+ "<p>Instead, you can access them through proto_toolchain</p>"
+ "<p><pre class=\"language-python\">\n"
+ "def _my_rule_impl(ctx):\n"
+ " proto_toolchain = ctx.toolchains[\"@rules_proto//proto:toolchain\"]\n"
+ "\n"
+ " # Contains the protoc binary, as specified by `--proto_compiler`.\n"
+ " protoc = proto_toolchain.compiler\n"
+ "\n"
+ " # Contains extra args to pass to protoc, as specified by `--protocopt`.\n"
+ " compiler_options = proto_toolchain.compiler_options\n"
+ "\n"
+ " # Contains the strict-dependency mode to use for Protocol Buffers\n"
+ " # (i.e. 'OFF`, `WARN`, `ERROR`), as specified by `--strict_proto_deps`.\n"
+ " strict_deps = proto_toolchain.strict_deps\n"
+ "\n"
+ "my_rule = rule(\n"
+ " implementation = _my_rule_impl,\n"
+ " attrs = {},\n"
+ " toolchains = [\n"
+ " \"@rules_proto//proto:toolchain\",\n"
+ " ],\n"
+ ")\n"
+ "</pre></p>"
)
public interface ProtoConfigurationApi extends StarlarkValue {
@SkylarkCallable(
// Must match the value of `_protocopt_key`
// in `@rules_proto//proto/private/rules:proto_toolchain.bzl`.
name = "protocopt_do_not_use_or_we_will_break_you_without_mercy",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hlopko , how about living without the scary "this is not a supported API" suffix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you like these suffices :) For my perspective I don't think this can cause too much trouble if we expose it under short name. And removing the option using an incompatible flag won't be too bad either. So feel free to use the short name Yannic, if you don't feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly either way. @lberki originally suggested them, so it's your call whether we use long or short names :). I'd like to keep the access them through proto_toolchain-suggestion, though.

doc = "Exposes the value of `--protocopt`."
+ "<p><b>Do not use this field directly</b>, its only purpose is to help with "
+ "migration of Protobuf rules to Starlark.</p>"
+ "<p>Instead, you can access the value through proto_toolchain</p>"
+ "<p><pre class=\"language-python\">\n"
+ "def _my_rule_impl(ctx):"
+ " proto_toolchain = ctx.toolchains[\"@rules_proto//proto:toolchain\"]\n"
+ " compiler_options = proto_toolchain.compiler_options\n"
+ "</pre></p>",
structField = true)
ImmutableList<String> protocOpts();

@SkylarkCallable(
// Must match the value of `_strict_deps_key`
// in `@rules_proto//proto/private/rules:proto_toolchain.bzl`.
name = "strict_deps_do_not_use_or_we_will_break_you_without_mercy",
Yannic marked this conversation as resolved.
Show resolved Hide resolved
doc = "Exposes the value of `--strict_proto_deps`."
+ "<p><b>Do not use this field directly</b>, its only purpose is to help with "
+ "migration of Protobuf rules to Starlark.</p>"
+ "<p>Instead, you can access the value through proto_toolchain</p>"
+ "<p><pre class=\"language-python\">\n"
+ "def _my_rule_impl(ctx):"
+ " proto_toolchain = ctx.toolchains[\"@rules_proto//proto:toolchain\"]\n"
+ " strict_deps = proto_toolchain.strict_deps\n"
+ "</pre></p>",
structField = true)
String starlarkStrictDeps();
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.skylarkbuildapi.proto;

import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.syntax.StarlarkValue;

Expand All @@ -31,4 +32,12 @@
+ "to load this symbol from <a href=\"https://github.com/bazelbuild/rules_proto\">"
+ "rules_proto</a>"
+ "</p>")
public interface ProtoModuleApi extends StarlarkValue {}
public interface ProtoModuleApi extends StarlarkValue {
@Deprecated
@SkylarkCallable(
name = "has_protoc_do_not_use_or_we_will_break_you_without_mercy",
doc = "Do not use this field, its only purpose is to help with migration of "
+ "Protobuf rules to Starlark.",
structField = true)
default void configurationFieldProtocExists() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this used for? To check for the presence of other fields? Then why not check directly for their presence instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to check whether --proto_compiler is available. Unfortunately, it's the only way to feature-detect late-bound defaults. https://github.com/bazelbuild/rules_proto/blob/919671ef0f73f5a82037eaabedc39c938c13cf60/proto/private/rules/proto_toolchain.bzl#L35

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative is to check for Bazel version (like we do e.g. here: https://github.com/bazelbuild/rules_rust/blob/master/rust/private/rustc.bzl#L138). That requires creating an external repository to write the bazel version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @laurentlb your thoughts on this? This is an example of users having to jump through hoops because we don't have a way to test if constant is defined?

}
11 changes: 11 additions & 0 deletions src/test/java/com/google/devtools/build/lib/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1421,6 +1421,17 @@ java_test(
],
)

java_test(
name = "ProtoConfigurationTest",
srcs = ["rules/proto/ProtoConfigurationTest.java"],
deps = [
":actions_testutil",
":analysis_testutil",
":guava_junit_truth",
"//src/main/java/com/google/devtools/build/lib/actions",
],
)

java_test(
name = "BazelProtoLibraryTest",
srcs = ["rules/proto/BazelProtoLibraryTest.java"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
// Copyright 2019 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.

package com.google.devtools.build.lib.rules.proto;

import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.getFirstArtifactEndingWith;

import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class ProtoConfigurationTest extends BuildViewTestCase {
@Before
public void setUp() throws Exception {
scratch.file("proto/BUILD", "cc_binary(name='compiler')");
scratch.file(
"proto/defs.bzl",
"def check_proto_common():",
" if not hasattr(proto_common,",
" 'has_protoc_do_not_use_or_we_will_break_you_without_mercy'):",
" fail('missing has_protoc_... attribute on proto_common')",
"check_proto_common()",
"",
"def _echo_protocopt_impl(ctx):",
" opts = ctx.fragments.proto.protocopt_do_not_use_or_we_will_break_you_without_mercy",
" f = ctx.actions.declare_file(ctx.attr.name)",
" ctx.actions.run(executable='echo',outputs=[f], arguments=opts)",
" return [DefaultInfo(files=depset([f]))]",
"echo_protocopt = rule(_echo_protocopt_impl, fragments=['proto'])",
"",
"def _echo_strict_deps_impl(ctx):",
" s = ctx.fragments.proto.strict_deps_do_not_use_or_we_will_break_you_without_mercy",
" f = ctx.actions.declare_file(ctx.attr.name)",
" ctx.actions.run(executable='echo',outputs=[f], arguments=[s])",
" return [DefaultInfo(files=depset([f]))]",
"echo_strict_deps = rule(_echo_strict_deps_impl, fragments=['proto'])",
"",
"def _echo_proto_compiler_impl(ctx):",
" return [DefaultInfo(files=depset([ctx.executable._compiler]))]",
"_protoc_key = 'protoc_do_not_use_or_we_will_break_you_without_mercy'",
"echo_proto_compiler = rule(",
" implementation = _echo_proto_compiler_impl,",
" attrs = {",
" '_compiler': attr.label(",
" executable = True,",
" cfg = 'host',",
" default = configuration_field('proto', _protoc_key),",
" ),",
" },",
" fragments=['proto']",
")");
}

private Artifact getArtifact(String target, String file) throws Exception {
return getFirstArtifactEndingWith(getFilesToBuild(getConfiguredTarget(target)), file);
}

@Test
public void readProtocopt() throws Exception {
scratch.file(
"x/BUILD",
"load('//proto:defs.bzl', 'echo_protocopt')",
"echo_protocopt(name = 'x')");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.isEmpty();

useConfiguration("--protocopt=--foo", "--protocopt=--bar=10");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("--foo", "--bar=10");
}

@Test
public void readStrictDeps() throws Exception {
scratch.file(
"x/BUILD",
"load('//proto:defs.bzl', 'echo_strict_deps')",
"echo_strict_deps(name = 'x')");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("ERROR");

useConfiguration("--strict_proto_deps=OFF");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("OFF");

useConfiguration("--strict_proto_deps=DEFAULT");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("OFF");

useConfiguration("--strict_proto_deps=WARN");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("WARN");

useConfiguration("--strict_proto_deps=ERROR");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("ERROR");

useConfiguration("--strict_proto_deps=DEFAULT");
assertThat(getGeneratingSpawnAction(getArtifact("//x", "x")).getRemainingArguments())
.containsExactly("OFF");
}

@Test
public void readProtoCompiler() throws Exception {
scratch.file(
"x/BUILD",
"load('//proto:defs.bzl', 'echo_proto_compiler')",
"echo_proto_compiler(name = 'x')");

useConfiguration("--proto_compiler=//proto:compiler");
assertThat(getArtifact("//x", "compiler").getRootRelativePathString())
.startsWith("proto/compiler"); // Ends with `.exe` on Windows.
}
}