Skip to content

Commit

Permalink
Extract execProperty handling to a dedicated class.
Browse files Browse the repository at this point in the history
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 568853694
Change-Id: I468d33b9eb0138e35274f7fd188c82ac851fbef4
  • Loading branch information
katre authored and copybara-github committed Sep 27, 2023
1 parent a4870a6 commit 612bf5c
Show file tree
Hide file tree
Showing 5 changed files with 258 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.StringUtilities;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import javax.annotation.Nullable;
import net.starlark.java.eval.Printer;
Expand Down Expand Up @@ -71,13 +69,13 @@ public class PlatformInfo extends NativeInfo
/** execProperties will deprecate and replace remoteExecutionProperties */
// TODO(blaze-configurability): If we want to remove remoteExecutionProperties, we need to fix
// PlatformUtils.getPlatformProto to use the dict-typed execProperties and do a migration.
private final ImmutableMap<String, String> execProperties;
private final PlatformProperties execProperties;

private PlatformInfo(
Label label,
ConstraintCollection constraints,
String remoteExecutionProperties,
ImmutableMap<String, String> execProperties,
PlatformProperties execProperties,
Location creationLocation) {
super(creationLocation);
this.label = label;
Expand Down Expand Up @@ -106,7 +104,7 @@ public String remoteExecutionProperties() {
}

public ImmutableMap<String, String> execProperties() {
return execProperties;
return execProperties.properties();
}

@Override
Expand All @@ -118,7 +116,7 @@ public void repr(Printer printer) {
public void addTo(Fingerprint fp) {
fp.addString(label.toString());
fp.addNullableString(remoteExecutionProperties);
fp.addStringMap(execProperties);
fp.addStringMap(execProperties.properties());
constraints.addToFingerprint(fp);
}

Expand Down Expand Up @@ -151,7 +149,7 @@ public static class Builder {
private Label label;
private final ConstraintCollection.Builder constraints = ConstraintCollection.builder();
private String remoteExecutionProperties = null;
@Nullable private ImmutableMap<String, String> execProperties;
private final PlatformProperties.Builder execPropertiesBuilder = PlatformProperties.builder();
private Location creationLocation = Location.BUILTIN;

/**
Expand All @@ -167,8 +165,10 @@ public Builder setParent(@Nullable PlatformInfo parent) {
this.parent = parent;
if (parent == null) {
this.constraints.parent(null);
this.execPropertiesBuilder.setParent(null);
} else {
this.constraints.parent(parent.constraints);
this.execPropertiesBuilder.setParent(parent.execProperties);
}
return this;
}
Expand Down Expand Up @@ -247,7 +247,7 @@ public Builder setRemoteExecutionProperties(String properties) {
*/
@CanIgnoreReturnValue
public Builder setExecProperties(@Nullable ImmutableMap<String, String> properties) {
this.execProperties = properties;
this.execPropertiesBuilder.setProperties(properties);
return this;
}

Expand All @@ -263,13 +263,17 @@ public Builder setLocation(Location location) {
return this;
}

private void checkRemoteExecutionProperties() throws ExecPropertiesException {
if (execProperties != null && !Strings.isNullOrEmpty(remoteExecutionProperties)) {
private static void checkRemoteExecutionProperties(
PlatformInfo parent,
String remoteExecutionProperties,
ImmutableMap<String, String> execProperties)
throws ExecPropertiesException {
if (!execProperties.isEmpty() && !Strings.isNullOrEmpty(remoteExecutionProperties)) {
throw new ExecPropertiesException(
"Platform contains both remote_execution_properties and exec_properties. Prefer"
+ " exec_properties over the deprecated remote_execution_properties.");
}
if (execProperties != null
if (!execProperties.isEmpty()
&& parent != null
&& !Strings.isNullOrEmpty(parent.remoteExecutionProperties())) {
throw new ExecPropertiesException(
Expand All @@ -295,20 +299,19 @@ private void checkRemoteExecutionProperties() throws ExecPropertiesException {
* constraint setting
*/
public PlatformInfo build() throws DuplicateConstraintException, ExecPropertiesException {
checkRemoteExecutionProperties();
checkRemoteExecutionProperties(
this.parent, this.remoteExecutionProperties, execPropertiesBuilder.getProperties());

// Merge the remote execution properties.
String remoteExecutionProperties =
mergeRemoteExecutionProperties(parent, this.remoteExecutionProperties);

ImmutableMap<String, String> execProperties =
mergeExecProperties(parent, this.execProperties);
if (execProperties == null) {
execProperties = ImmutableMap.of();
}

return new PlatformInfo(
label, constraints.build(), remoteExecutionProperties, execProperties, creationLocation);
label,
constraints.build(),
remoteExecutionProperties,
execPropertiesBuilder.build(),
creationLocation);
}

private static String mergeRemoteExecutionProperties(
Expand All @@ -325,31 +328,6 @@ private static String mergeRemoteExecutionProperties(
return StringUtilities.replaceAllLiteral(
remoteExecutionProperties, PARENT_REMOTE_EXECUTION_KEY, parentRemoteExecutionProperties);
}

@Nullable
private static ImmutableMap<String, String> mergeExecProperties(
PlatformInfo parent, Map<String, String> execProperties) {
if ((parent == null || parent.execProperties() == null) && execProperties == null) {
return null;
}

HashMap<String, String> result = new HashMap<>();
if (parent != null && parent.execProperties() != null) {
result.putAll(parent.execProperties());
}

if (execProperties != null) {
for (Map.Entry<String, String> entry : execProperties.entrySet()) {
if (Strings.isNullOrEmpty(entry.getValue())) {
result.remove(entry.getKey());
} else {
result.put(entry.getKey(), entry.getValue());
}
}
}

return ImmutableMap.copyOf(result);
}
}

/** Exception that indicates something is wrong in exec_properties configuration. */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright 2023 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.analysis.platform;

import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;

/** Proepeties set on a specific {@link PlatformInfo}. */
@AutoValue
public abstract class PlatformProperties {
abstract ImmutableMap<String, String> properties();

public boolean isEmpty() {
return properties().isEmpty();
}

/** Returns a new {@link Builder} for creating a fresh {@link PlatformProperties} instance. */
public static Builder builder() {
return new Builder();
}

/** Builder class to facilitate creating valid {@link PlatformProperties} instances. */
public static final class Builder {
@Nullable private PlatformProperties parent = null;
private ImmutableMap<String, String> properties = ImmutableMap.of();

@CanIgnoreReturnValue
public Builder setParent(@Nullable PlatformProperties parent) {
this.parent = parent;
return this;
}

/** Returns the current properties (but not any from the parent), for validation. */
ImmutableMap<String, String> getProperties() {
return this.properties;
}

@CanIgnoreReturnValue
public Builder setProperties(Map<String, String> properties) {
this.properties = ImmutableMap.copyOf(properties);
return this;
}

public PlatformProperties build() {
ImmutableMap<String, String> properties = mergeParent(parent, this.properties);

return new AutoValue_PlatformProperties(properties);
}

@Nullable
private static ImmutableMap<String, String> mergeParent(
PlatformProperties parent, ImmutableMap<String, String> properties) {
if (parent == null || parent.isEmpty()) {
return properties;
}

HashMap<String, String> result = new HashMap<>();
if (parent != null && !parent.properties().isEmpty()) {
result.putAll(parent.properties());
}

if (!properties.isEmpty()) {
for (Map.Entry<String, String> entry : properties.entrySet()) {
if (Strings.isNullOrEmpty(entry.getValue())) {
result.remove(entry.getKey());
} else {
result.put(entry.getKey(), entry.getValue());
}
}
}

return ImmutableMap.copyOf(result);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@
// 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.analysis.platform;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableMap;
import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo.ExecPropertiesException;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import org.junit.Test;
Expand Down Expand Up @@ -238,6 +238,27 @@ public void remoteExecutionProperties_parentPlatform_merge_parentNotSet() throws
assertThat(platformInfo.remoteExecutionProperties()).isEqualTo("child properties");
}

@Test
public void remoteExecutionProperties_parentSpecifiesExecProperties_error() throws Exception {
PlatformInfo parent =
PlatformInfo.builder()
.setLabel(Label.parseCanonicalUnchecked("//foo:parent_platform"))
.setExecProperties(ImmutableMap.of("elem1", "value1"))
.build();

PlatformInfo.Builder builder = PlatformInfo.builder();
builder.setParent(parent);
builder.setRemoteExecutionProperties("props");

ExecPropertiesException exception = assertThrows(ExecPropertiesException.class, builder::build);
assertThat(exception)
.hasMessageThat()
.contains(
"Platform specifies remote_execution_properties but its parent specifies"
+ " exec_properties. Prefer exec_properties over the deprecated"
+ " remote_execution_properties.");
}

@Test
public void execProperties_empty() throws Exception {
PlatformInfo.Builder builder = PlatformInfo.builder();
Expand Down Expand Up @@ -290,6 +311,41 @@ public void execProperties_parentPlatform_inheritance() throws Exception {
assertThat(platformInfo.execProperties()).containsExactly("p1", "keep", "p3", "child");
}

@Test
public void execProperties_remoteExecProperties_error() throws Exception {
PlatformInfo.Builder builder = PlatformInfo.builder();
builder.setExecProperties(ImmutableMap.of("elem1", "value1"));
builder.setRemoteExecutionProperties("props");

ExecPropertiesException exception = assertThrows(ExecPropertiesException.class, builder::build);
assertThat(exception)
.hasMessageThat()
.contains(
"Platform contains both remote_execution_properties and exec_properties. Prefer"
+ " exec_properties over the deprecated remote_execution_properties.");
}

@Test
public void execProperties_parentSpecifiesRemoteExecutionProperties_error() throws Exception {
PlatformInfo parent =
PlatformInfo.builder()
.setLabel(Label.parseCanonicalUnchecked("//foo:parent_platform"))
.setRemoteExecutionProperties("props")
.build();

PlatformInfo.Builder builder = PlatformInfo.builder();
builder.setParent(parent);
builder.setExecProperties(ImmutableMap.of("elem1", "value1"));

ExecPropertiesException exception = assertThrows(ExecPropertiesException.class, builder::build);
assertThat(exception)
.hasMessageThat()
.contains(
"Platform specifies exec_properties but its parent //foo:parent_platform specifies"
+ " remote_execution_properties. Prefer exec_properties over the deprecated"
+ " remote_execution_properties.");
}

@Test
public void equalsTester() throws Exception {
ConstraintSettingInfo setting1 =
Expand Down
Loading

0 comments on commit 612bf5c

Please sign in to comment.