Skip to content

Commit 78af34f

Browse files
comiuskotlajawilwellckolli5
authored
Cherry-pick proto_lang_toolchain Starlarkfication and proto_common module (bazelbuild#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 bazelbuild@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 <[email protected]> Co-authored-by: wilwell <[email protected]> Co-authored-by: Chenchu K <[email protected]>
1 parent 40e485d commit 78af34f

File tree

61 files changed

+2312
-347
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

61 files changed

+2312
-347
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionExecutionException.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.devtools.build.lib.skyframe.DetailedException;
2525
import com.google.devtools.build.lib.util.DetailedExitCode;
2626
import com.google.devtools.build.lib.util.ExitCode;
27+
import javax.annotation.Nullable;
2728
import net.starlark.java.syntax.Location;
2829

2930
/**
@@ -115,6 +116,47 @@ private static NestedSet<Cause> rootCausesFromAction(
115116
detailedExitCode));
116117
}
117118

119+
public static ActionExecutionException fromExecException(ExecException exception, Action action) {
120+
return fromExecException(exception, null, action);
121+
}
122+
123+
/**
124+
* Returns a new ActionExecutionException given an optional action subtask describing which part
125+
* of the action failed (should be null for standard action failures). When appropriate (we use
126+
* some heuristics to decide), produces an abbreviated message incorporating just the termination
127+
* status if available.
128+
*
129+
* @param exception initial ExecException
130+
* @param actionSubtask additional information about the action
131+
* @param action failed action
132+
* @return ActionExecutionException object describing the action failure
133+
*/
134+
public static ActionExecutionException fromExecException(
135+
ExecException exception, @Nullable String actionSubtask, Action action) {
136+
// Message from ActionExecutionException will be prepended with action.describe() where
137+
// necessary: because not all ActionExecutionExceptions come from this codepath, it is safer
138+
// for consumers to manually prepend. We still put action.describe() in the failure detail
139+
// message argument.
140+
String message =
141+
(actionSubtask == null ? "" : actionSubtask + ": ")
142+
+ exception.getMessageForActionExecutionException();
143+
144+
DetailedExitCode code =
145+
DetailedExitCode.of(exception.getFailureDetail(action.describe() + " failed: " + message));
146+
147+
if (exception instanceof LostInputsExecException) {
148+
return ((LostInputsExecException) exception).fromExecException(message, action, code);
149+
}
150+
151+
return fromExecException(exception, message, action, code);
152+
}
153+
154+
public static ActionExecutionException fromExecException(
155+
ExecException exception, String message, Action action, DetailedExitCode code) {
156+
return new ActionExecutionException(
157+
message, exception, action, exception.isCatastrophic(), code);
158+
}
159+
118160
/** Returns the action that failed. */
119161
public ActionAnalysisMetadata getAction() {
120162
return action;

‎src/main/java/com/google/devtools/build/lib/actions/BUILD‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,7 @@ java_library(
297297
"ResourceSetOrBuilder.java",
298298
],
299299
deps = [
300-
":artifacts",
301-
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
300+
":exec_exception",
302301
"//src/main/java/com/google/devtools/build/lib/concurrent",
303302
"//src/main/java/com/google/devtools/build/lib/jni",
304303
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
@@ -344,3 +343,12 @@ java_library(
344343
"//third_party:jsr305",
345344
],
346345
)
346+
347+
java_library(
348+
name = "exec_exception",
349+
srcs = [
350+
"ExecException.java",
351+
"UserExecException.java",
352+
],
353+
deps = ["//src/main/protobuf:failure_details_java_proto"],
354+
)

‎src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.common.collect.Iterables;
2121
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
2222
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
23+
import com.google.devtools.build.lib.util.OS;
2324
import com.google.devtools.build.lib.vfs.PathFragment;
2425
import java.util.List;
2526
import java.util.Map;
@@ -35,15 +36,15 @@ public class BaseSpawn implements Spawn {
3536
private final ImmutableMap<String, String> executionInfo;
3637
private final RunfilesSupplier runfilesSupplier;
3738
private final ActionExecutionMetadata action;
38-
private final ResourceSet localResources;
39+
private final ResourceSetOrBuilder localResources;
3940

4041
public BaseSpawn(
4142
List<String> arguments,
4243
Map<String, String> environment,
4344
Map<String, String> executionInfo,
4445
RunfilesSupplier runfilesSupplier,
4546
ActionExecutionMetadata action,
46-
ResourceSet localResources) {
47+
ResourceSetOrBuilder localResources) {
4748
this.arguments = ImmutableList.copyOf(arguments);
4849
this.environment = ImmutableMap.copyOf(environment);
4950
this.executionInfo = ImmutableMap.copyOf(executionInfo);
@@ -123,8 +124,9 @@ public ActionExecutionMetadata getResourceOwner() {
123124
}
124125

125126
@Override
126-
public ResourceSet getLocalResources() {
127-
return localResources;
127+
public ResourceSet getLocalResources() throws ExecException {
128+
return localResources.buildResourceSet(
129+
OS.getCurrent(), action.getInputs().memoizedFlattenAndGetSize());
128130
}
129131

130132
@Override

‎src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public ActionExecutionMetadata getResourceOwner() {
8484
}
8585

8686
@Override
87-
public ResourceSet getLocalResources() {
87+
public ResourceSet getLocalResources() throws ExecException {
8888
return spawn.getLocalResources();
8989
}
9090

‎src/main/java/com/google/devtools/build/lib/actions/ExecException.java‎

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,6 @@
1515
package com.google.devtools.build.lib.actions;
1616

1717
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
18-
import com.google.devtools.build.lib.util.DetailedExitCode;
19-
import com.google.errorprone.annotations.ForOverride;
20-
import javax.annotation.Nullable;
2118

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

85-
/**
86-
* Returns a new ActionExecutionException without a message prefix.
87-
*
88-
* @param action failed action
89-
* @return ActionExecutionException object describing the action failure
90-
*/
91-
public final ActionExecutionException toActionExecutionException(Action action) {
92-
return toActionExecutionException(null, action);
93-
}
94-
95-
/**
96-
* Returns a new ActionExecutionException given an optional action subtask describing which part
97-
* of the action failed (should be null for standard action failures). When appropriate (we use
98-
* some heuristics to decide), produces an abbreviated message incorporating just the termination
99-
* status if available.
100-
*
101-
* @param actionSubtask additional information about the action
102-
* @param action failed action
103-
* @return ActionExecutionException object describing the action failure
104-
*/
105-
public final ActionExecutionException toActionExecutionException(
106-
@Nullable String actionSubtask, Action action) {
107-
// Message from ActionExecutionException will be prepended with action.describe() where
108-
// necessary: because not all ActionExecutionExceptions come from this codepath, it is safer
109-
// for consumers to manually prepend. We still put action.describe() in the failure detail
110-
// message argument.
111-
String message =
112-
(actionSubtask == null ? "" : actionSubtask + ": ")
113-
+ getMessageForActionExecutionException();
114-
return toActionExecutionException(
115-
message,
116-
action,
117-
DetailedExitCode.of(getFailureDetail(action.describe() + " failed: " + message)));
118-
}
119-
120-
@ForOverride
121-
protected ActionExecutionException toActionExecutionException(
122-
String message, Action action, DetailedExitCode code) {
123-
return new ActionExecutionException(message, this, action, isCatastrophic(), code);
124-
}
125-
126-
@ForOverride
12782
protected String getMessageForActionExecutionException() {
12883
return getMessage();
12984
}
13085

131-
@ForOverride
13286
protected abstract FailureDetail getFailureDetail(String message);
13387
}

‎src/main/java/com/google/devtools/build/lib/actions/LostInputsExecException.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,7 @@ public ActionInputDepOwners getOwners() {
6464
return owners;
6565
}
6666

67-
@Override
68-
protected ActionExecutionException toActionExecutionException(
67+
protected ActionExecutionException fromExecException(
6968
String message, Action action, DetailedExitCode code) {
7069
return new LostInputsActionExecutionException(
7170
message, lostInputs, owners, action, /*cause=*/ this, code);

‎src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@
1616

1717
import com.google.common.base.Splitter;
1818
import com.google.common.primitives.Doubles;
19-
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2019
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
20+
import com.google.devtools.build.lib.util.OS;
2121
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
2222
import com.google.devtools.common.options.Converter;
2323
import com.google.devtools.common.options.OptionsParsingException;
@@ -175,7 +175,7 @@ public String getTypeDescription() {
175175
}
176176

177177
@Override
178-
public ResourceSet buildResourceSet(NestedSet<Artifact> inputs) {
178+
public ResourceSet buildResourceSet(OS os, int inputsSize) throws ExecException {
179179
return this;
180180
}
181181
}

‎src/main/java/com/google/devtools/build/lib/actions/ResourceSetOrBuilder.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

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

17-
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
17+
import com.google.devtools.build.lib.util.OS;
1818

1919
/** Common interface for ResourceSet and builder. */
2020
@FunctionalInterface
@@ -24,5 +24,5 @@ public interface ResourceSetOrBuilder {
2424
* will flatten NestedSet. This action could create a lot of garbagge, so use it as close as
2525
* possible to the execution phase,
2626
*/
27-
public ResourceSet buildResourceSet(NestedSet<Artifact> inputs);
27+
public ResourceSet buildResourceSet(OS os, int inputsSize) throws ExecException;
2828
}

‎src/main/java/com/google/devtools/build/lib/actions/Spawn.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,8 @@ default boolean isMandatoryOutput(ActionInput output) {
130130
/** Returns the resource owner for local fallback. */
131131
ActionExecutionMetadata getResourceOwner();
132132

133-
/**
134-
* Returns the amount of resources needed for local fallback.
135-
*/
136-
ResourceSet getLocalResources();
133+
/** Returns the amount of resources needed for local fallback. */
134+
ResourceSet getLocalResources() throws ExecException;
137135

138136
/**
139137
* Returns a mnemonic (string constant) for this kind of spawn.

‎src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,16 +89,14 @@ public ActionContinuationOrResult execute()
8989
return this;
9090
}
9191
} catch (ExecException e) {
92-
throw e.toActionExecutionException(
93-
AbstractFileWriteAction.this);
92+
throw ActionExecutionException.fromExecException(e, AbstractFileWriteAction.this);
9493
}
9594
afterWrite(actionExecutionContext);
9695
return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
9796
}
9897
};
9998
} catch (ExecException e) {
100-
throw e.toActionExecutionException(
101-
this);
99+
throw ActionExecutionException.fromExecException(e, this);
102100
}
103101
}
104102

0 commit comments

Comments
 (0)