Skip to content

Commit

Permalink
Cherry-pick proto_lang_toolchain Starlarkfication and proto_common mo…
Browse files Browse the repository at this point in the history
…dule (#15854)

* Put protoc label to a constant.

Renamed StrictProtoDepsViolationMessage to ProtoConstants and added protoc label there.

PiperOrigin-RevId: 409142099

* Remove proto_lang_toolchain rule's $(PLUGIN_OUT) placeholder and simplify ProtoCompileActionBuilder.

`$(OUT)` placeholder is replaced with `%s` so it can be directly used in addFormatted. This simplifies construction of proto compile action and makes it possible for Starlark version to have the same performance.

github shows no uses of the placeholder: https://github.com/search?q=%22%24%28PLUGIN_OUT%29%22&type=Repositories

PiperOrigin-RevId: 412286743

* Add plugin_format_flag attribute to ProtoLangToolchainRule

This makes it possible to pair it with command_line attribute which could contain dependent data,
for example:

```
proto_lang_toolchain(
    name = "j2objc_proto_toolchain",
    blacklisted_protos = [],
    command_line = "--PLUGIN_j2objc_out=file_dir_mapping,generate_class_mappings:$(OUT)",
+   plugin_format_flag = "--plugin=protoc-gen-PLUGIN_j2objc=%s",
    plugin = "//third_party/java/j2objc:proto_plugin",
    runtime = "//third_party/java/j2objc:proto_runtime",
    visibility = ["//visibility:public"],
)
```

It also retains reference to the flag on proto_lang_toolchain rule and so doesn't cause a memory regression when proto_lang_libraries are Starlarkyfied.

PiperOrigin-RevId: 412287195

* Proxy proto compiler and proto opts over proto_lang_toolchain rule.

This is needed for the proto_common.generate_code function.

This change doesn't modify any of proto_lang_toolchain public attributes.

PiperOrigin-RevId: 437713619

* Extend proto_lang_toolchain rule with progress_message and mnemonic attributes.

This is needed for proto_common.generate_sources function.

Two public attributes are added to proto_lang_toolchain rule. Both have defaults, so no immediate migration of targets is needed.

PiperOrigin-RevId: 437714094

* Starlarkify proto_lang_toolchain and ProtoLangToolchainInfo provider

Added StarlarkProtoLangToolchainTest which uses starlarkified rule for verification.

I've deleted blacklisted_protos and forbidden_protos since they are not needed anymore.

PiperOrigin-RevId: 444223497

* Roll forward of ae349e9: Export ProtoLangToolchainInfo provider and flip proto_lang_toolchain rule

NEW: I've moved ProtoLangToolchainProvider from providers.bzl file to proto_common.bzl file since proto_common doesn't allow loading other components. I've also deleted providers.bzl file since we don't need it anymore.

Automated rollback of commit 5a6b1a8.

*** Reason for rollback ***

Good to submit since the blocking error has been resolved.

*** Original change description ***

Automated rollback of commit ae349e9.

*** Reason for rollback ***

This CL breaks proto_common.bzl file which seems that can't load providers.bzl file.

*** Original change description ***

Export ProtoLangToolchainInfo provider and flip proto_lang_toolchain rule

I’ve exported ProtoLangToolchainInfo provider from it’s native class by adding two new functions: one that’s creating starlark provider (create function), and the other that’s wrapping the starlark provider as a nat

***

PiperOrigin-RevId: 446401388

* Use data from transitive_proto_sources instead of transitive_sources in proto_lang_toolchain.

The problem with latter is, that transitive_source can contain "renamed" files (in _virtual_includes subdirectory), which doesn't work for the detection that needs original files (ProtoSource.original_source_file).

PiperOrigin-RevId: 446924924

* Remove allow_files from proto_lang_toolchain's attributes

It's the same behaviour in native code.

PiperOrigin-RevId: 446988764

* Remove native implementation of proto_lang_toolchain rule

PiperOrigin-RevId: 446995789

* Fix name in proto_lang_toolchain rule

Added a wrapper for proto_lang_toolchain in order to have two implementations - one with public proto_compiler attribute, and the other one with the private one.
Restructured proto_lang_toolchain rules' implementation - merged two rule's definitions into one.

PiperOrigin-RevId: 447016335

* Cherry-pick missing things.

* Separate ExecException.java from main actions target.

PiperOrigin-RevId: 413105213

* ResourceSet in StarlarkAction API

Added optional `resource_set` parameter to `run` and `run_shell` in StarlarkActionApi. `resource_set` is `StarlarkCallable` object that returns dict with resource set (cpu, memory, local_test).

PiperOrigin-RevId: 415224490

* Implement and expose proto_common.compile call.

Design doc: https://docs.google.com/document/d/1dY_jfRvnH8SjRXGIfg8av-vquyWsvIZydXJOywvaR1A/edit

PiperOrigin-RevId: 440098122

* Fix ProtoCommon tests.

* Add experimental_progress_message parameter to proto_common.compile.

This will make migration to the new call easier (because we don't have progress_message set yet on the proto_lang_toolchain rules).

Design doc: https://docs.google.com/document/d/1dY_jfRvnH8SjRXGIfg8av-vquyWsvIZydXJOywvaR1A/edit

PiperOrigin-RevId: 440098602

* Implement proto_common.experimental_should_generate_code.

Design doc: https://docs.google.com/document/d/1dY_jfRvnH8SjRXGIfg8av-vquyWsvIZydXJOywvaR1A/edit

PiperOrigin-RevId: 440109298

* Implement proto_common.declare_generated_files.

Design doc: https://docs.google.com/document/d/1dY_jfRvnH8SjRXGIfg8av-vquyWsvIZydXJOywvaR1A/edit

PiperOrigin-RevId: 441097041

* Fix docstring in proto_common

PiperOrigin-RevId: 445149402

* Add proto_info parameter to proto_common.compile

Adding it will make it possible to migrate the uses from proto_library_target to proto_info. When the migration is done proto_library_target will be removed.

The cost of changing this now is still low, because it's not yet released/used in Bazel.

PiperOrigin-RevId: 455616355
Change-Id: Ieb0f03b0600e1f90b72a61f90420675075c79a9e

* Migrate proto_library_target to proto_info in proto_common.declare_generated_files.

PiperOrigin-RevId: 456475071
Change-Id: I2882d80cd4f7fdd9b8dcba3347930eaf1f194d0a

* Migrate proto_library_target to proto_info in proto_common.experimental_should_generate_code

PiperOrigin-RevId: 460162832
Change-Id: I57a6fa4c6e6c9618cf9edb8518e17b46fc90be9f

* Remove proto_library_target from proto_common

PiperOrigin-RevId: 460406536
Change-Id: I10021f32fb40e163ded02ebab8297902b63760fa

Co-authored-by: kotlaja <kotlaja@google.com>
Co-authored-by: wilwell <wilwell@google.com>
Co-authored-by: Chenchu K <ckolli@google.com>
  • Loading branch information
4 people authored Jul 14, 2022
1 parent 40e485d commit 78af34f
Show file tree
Hide file tree
Showing 61 changed files with 2,312 additions and 347 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.devtools.build.lib.skyframe.DetailedException;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.ExitCode;
import javax.annotation.Nullable;
import net.starlark.java.syntax.Location;

/**
Expand Down Expand Up @@ -115,6 +116,47 @@ private static NestedSet<Cause> rootCausesFromAction(
detailedExitCode));
}

public static ActionExecutionException fromExecException(ExecException exception, Action action) {
return fromExecException(exception, null, action);
}

/**
* Returns a new ActionExecutionException given an optional action subtask describing which part
* of the action failed (should be null for standard action failures). When appropriate (we use
* some heuristics to decide), produces an abbreviated message incorporating just the termination
* status if available.
*
* @param exception initial ExecException
* @param actionSubtask additional information about the action
* @param action failed action
* @return ActionExecutionException object describing the action failure
*/
public static ActionExecutionException fromExecException(
ExecException exception, @Nullable String actionSubtask, Action action) {
// Message from ActionExecutionException will be prepended with action.describe() where
// necessary: because not all ActionExecutionExceptions come from this codepath, it is safer
// for consumers to manually prepend. We still put action.describe() in the failure detail
// message argument.
String message =
(actionSubtask == null ? "" : actionSubtask + ": ")
+ exception.getMessageForActionExecutionException();

DetailedExitCode code =
DetailedExitCode.of(exception.getFailureDetail(action.describe() + " failed: " + message));

if (exception instanceof LostInputsExecException) {
return ((LostInputsExecException) exception).fromExecException(message, action, code);
}

return fromExecException(exception, message, action, code);
}

public static ActionExecutionException fromExecException(
ExecException exception, String message, Action action, DetailedExitCode code) {
return new ActionExecutionException(
message, exception, action, exception.isCatastrophic(), code);
}

/** Returns the action that failed. */
public ActionAnalysisMetadata getAction() {
return action;
Expand Down
12 changes: 10 additions & 2 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,7 @@ java_library(
"ResourceSetOrBuilder.java",
],
deps = [
":artifacts",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
":exec_exception",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/jni",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
Expand Down Expand Up @@ -344,3 +343,12 @@ java_library(
"//third_party:jsr305",
],
)

java_library(
name = "exec_exception",
srcs = [
"ExecException.java",
"UserExecException.java",
],
deps = ["//src/main/protobuf:failure_details_java_proto"],
)
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.List;
import java.util.Map;
Expand All @@ -35,15 +36,15 @@ public class BaseSpawn implements Spawn {
private final ImmutableMap<String, String> executionInfo;
private final RunfilesSupplier runfilesSupplier;
private final ActionExecutionMetadata action;
private final ResourceSet localResources;
private final ResourceSetOrBuilder localResources;

public BaseSpawn(
List<String> arguments,
Map<String, String> environment,
Map<String, String> executionInfo,
RunfilesSupplier runfilesSupplier,
ActionExecutionMetadata action,
ResourceSet localResources) {
ResourceSetOrBuilder localResources) {
this.arguments = ImmutableList.copyOf(arguments);
this.environment = ImmutableMap.copyOf(environment);
this.executionInfo = ImmutableMap.copyOf(executionInfo);
Expand Down Expand Up @@ -123,8 +124,9 @@ public ActionExecutionMetadata getResourceOwner() {
}

@Override
public ResourceSet getLocalResources() {
return localResources;
public ResourceSet getLocalResources() throws ExecException {
return localResources.buildResourceSet(
OS.getCurrent(), action.getInputs().memoizedFlattenAndGetSize());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public ActionExecutionMetadata getResourceOwner() {
}

@Override
public ResourceSet getLocalResources() {
public ResourceSet getLocalResources() throws ExecException {
return spawn.getLocalResources();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@
package com.google.devtools.build.lib.actions;

import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.errorprone.annotations.ForOverride;
import javax.annotation.Nullable;

/**
* An exception indication that the execution of an action has failed OR could not be attempted OR
Expand Down Expand Up @@ -82,52 +79,9 @@ public boolean isCatastrophic() {
return catastrophe;
}

/**
* Returns a new ActionExecutionException without a message prefix.
*
* @param action failed action
* @return ActionExecutionException object describing the action failure
*/
public final ActionExecutionException toActionExecutionException(Action action) {
return toActionExecutionException(null, action);
}

/**
* Returns a new ActionExecutionException given an optional action subtask describing which part
* of the action failed (should be null for standard action failures). When appropriate (we use
* some heuristics to decide), produces an abbreviated message incorporating just the termination
* status if available.
*
* @param actionSubtask additional information about the action
* @param action failed action
* @return ActionExecutionException object describing the action failure
*/
public final ActionExecutionException toActionExecutionException(
@Nullable String actionSubtask, Action action) {
// Message from ActionExecutionException will be prepended with action.describe() where
// necessary: because not all ActionExecutionExceptions come from this codepath, it is safer
// for consumers to manually prepend. We still put action.describe() in the failure detail
// message argument.
String message =
(actionSubtask == null ? "" : actionSubtask + ": ")
+ getMessageForActionExecutionException();
return toActionExecutionException(
message,
action,
DetailedExitCode.of(getFailureDetail(action.describe() + " failed: " + message)));
}

@ForOverride
protected ActionExecutionException toActionExecutionException(
String message, Action action, DetailedExitCode code) {
return new ActionExecutionException(message, this, action, isCatastrophic(), code);
}

@ForOverride
protected String getMessageForActionExecutionException() {
return getMessage();
}

@ForOverride
protected abstract FailureDetail getFailureDetail(String message);
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ public ActionInputDepOwners getOwners() {
return owners;
}

@Override
protected ActionExecutionException toActionExecutionException(
protected ActionExecutionException fromExecException(
String message, Action action, DetailedExitCode code) {
return new LostInputsActionExecutionException(
message, lostInputs, owners, action, /*cause=*/ this, code);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

import com.google.common.base.Splitter;
import com.google.common.primitives.Doubles;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.OptionsParsingException;
Expand Down Expand Up @@ -175,7 +175,7 @@ public String getTypeDescription() {
}

@Override
public ResourceSet buildResourceSet(NestedSet<Artifact> inputs) {
public ResourceSet buildResourceSet(OS os, int inputsSize) throws ExecException {
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

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

import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.util.OS;

/** Common interface for ResourceSet and builder. */
@FunctionalInterface
Expand All @@ -24,5 +24,5 @@ public interface ResourceSetOrBuilder {
* will flatten NestedSet. This action could create a lot of garbagge, so use it as close as
* possible to the execution phase,
*/
public ResourceSet buildResourceSet(NestedSet<Artifact> inputs);
public ResourceSet buildResourceSet(OS os, int inputsSize) throws ExecException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,8 @@ default boolean isMandatoryOutput(ActionInput output) {
/** Returns the resource owner for local fallback. */
ActionExecutionMetadata getResourceOwner();

/**
* Returns the amount of resources needed for local fallback.
*/
ResourceSet getLocalResources();
/** Returns the amount of resources needed for local fallback. */
ResourceSet getLocalResources() throws ExecException;

/**
* Returns a mnemonic (string constant) for this kind of spawn.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,14 @@ public ActionContinuationOrResult execute()
return this;
}
} catch (ExecException e) {
throw e.toActionExecutionException(
AbstractFileWriteAction.this);
throw ActionExecutionException.fromExecException(e, AbstractFileWriteAction.this);
}
afterWrite(actionExecutionContext);
return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
}
};
} catch (ExecException e) {
throw e.toActionExecutionException(
this);
throw ActionExecutionException.fromExecException(e, this);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public final ActionContinuationOrResult beginExecution(
beforeExecute(actionExecutionContext);
spawn = getSpawn(actionExecutionContext);
} catch (ExecException e) {
throw e.toActionExecutionException(this);
throw ActionExecutionException.fromExecException(e, this);
} catch (CommandLineExpansionException e) {
throw createCommandLineException(e);
}
Expand Down Expand Up @@ -590,8 +590,7 @@ private ActionSpawn(
executionInfo,
SpawnAction.this.getRunfilesSupplier(),
SpawnAction.this,
SpawnAction.this.resourceSetOrBuilder.buildResourceSet(inputs));

SpawnAction.this.resourceSetOrBuilder);
NestedSetBuilder<ActionInput> inputsBuilder = NestedSetBuilder.stableOrder();
ImmutableList<Artifact> manifests = getRunfilesSupplier().getManifests();
for (Artifact input : inputs.toList()) {
Expand Down Expand Up @@ -1428,7 +1427,7 @@ public ActionContinuationOrResult execute()
}
return new SpawnActionContinuation(actionExecutionContext, nextContinuation);
} catch (ExecException e) {
throw e.toActionExecutionException(SpawnAction.this);
throw ActionExecutionException.fromExecException(e, SpawnAction.this);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,7 @@ public ActionContinuationOrResult execute()
return this;
}
} catch (ExecException e) {
throw e.toActionExecutionException(
TemplateExpansionAction.this);
throw ActionExecutionException.fromExecException(e, TemplateExpansionAction.this);
}
return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
}
Expand Down
Loading

0 comments on commit 78af34f

Please sign in to comment.