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.

Sign up for GitHub

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

Jump to bottom

Normalize C++ action config tool paths based on exec platform OS #22218

Closed
wants to merge 1 commit into from
Closed

Normalize C++ action config tool paths based on exec platform OS #22218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,17 @@ public boolean isExecutedOnWindows() {
.hasConstraintValue(OS_TO_CONSTRAINTS.get(OS.WINDOWS));
}

/** Returns the OS of the execution platform. */
public OS getExecutionPlatformOs() {
for (var osToConstraint : OS_TO_CONSTRAINTS.entrySet()) {
if (getExecutionPlatform().constraints().hasConstraintValue(osToConstraint.getValue())) {
return osToConstraint.getKey();
}
}
// Fall back to assuming exec OS == host OS.
return OS.getCurrent();
}

/**
* @return the set of features applicable for the current rule.
*/
Expand Down
46 changes: 19 additions & 27 deletions src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,33 +30,25 @@ public final class ConstraintConstants {

// Standard mapping between OS and the corresponding platform constraints.
public static final ImmutableMap<OS, ConstraintValueInfo> OS_TO_CONSTRAINTS =
ImmutableMap.<OS, ConstraintValueInfo>builder()
.put(
OS.DARWIN,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING,
Label.parseCanonicalUnchecked("@platforms//os:osx")))
.put(
OS.WINDOWS,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING,
Label.parseCanonicalUnchecked("@platforms//os:windows")))
.put(
OS.FREEBSD,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING,
Label.parseCanonicalUnchecked("@platforms//os:freebsd")))
.put(
OS.OPENBSD,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING,
Label.parseCanonicalUnchecked("@platforms//os:openbsd")))
.put(
OS.UNKNOWN,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING,
Label.parseCanonicalUnchecked("@platforms//os:none")))
.buildOrThrow();
ImmutableMap.of(
OS.LINUX,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:linux")),
OS.DARWIN,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:osx")),
OS.WINDOWS,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:windows")),
OS.FREEBSD,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:freebsd")),
OS.OPENBSD,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:openbsd")),
OS.UNKNOWN,
ConstraintValueInfo.create(
OS_CONSTRAINT_SETTING, Label.parseCanonicalUnchecked("@platforms//os:none")));

// No-op constructor to keep this from being instantiated.
private ConstraintConstants() {}
Expand Down
18 changes: 13 additions & 5 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
import com.google.devtools.build.lib.starlarkbuildapi.cpp.CcModuleApi;
import com.google.devtools.build.lib.starlarkbuildapi.cpp.ExtraLinkTimeLibraryApi;
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.StringUtil;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -1205,10 +1206,11 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromStarlark(
ImmutableSet<String> featureNames =
featureList.stream().map(Feature::getName).collect(toImmutableSet());

OS execOs = starlarkRuleContext.getRuleContext().getExecutionPlatformOs();
ImmutableList.Builder<ActionConfig> actionConfigBuilder = ImmutableList.builder();
for (Object actionConfig : actionConfigs) {
checkRightStarlarkInfoProvider(actionConfig, "action_configs", "ActionConfigInfo");
actionConfigBuilder.add(actionConfigFromStarlark((StarlarkInfo) actionConfig));
actionConfigBuilder.add(actionConfigFromStarlark((StarlarkInfo) actionConfig, execOs));
}
ImmutableList<ActionConfig> actionConfigList = actionConfigBuilder.build();

Expand Down Expand Up @@ -1676,7 +1678,8 @@ static FlagSet flagSetFromStarlark(StarlarkInfo flagSetStruct, String actionName
* {@link StarlarkInfo}.
*/
@VisibleForTesting
static CcToolchainFeatures.Tool toolFromStarlark(StarlarkInfo toolStruct) throws EvalException {
static CcToolchainFeatures.Tool toolFromStarlark(StarlarkInfo toolStruct, OS execOs)
throws EvalException {
checkRightProviderType(toolStruct, "tool");

String toolPathString = getOptionalFieldFromStarlarkProvider(toolStruct, "path", String.class);
Expand All @@ -1690,7 +1693,12 @@ static CcToolchainFeatures.Tool toolFromStarlark(StarlarkInfo toolStruct) throws
throw infoError(toolStruct, "\"tool\" and \"path\" cannot be set at the same time.");
}

toolPath = PathFragment.create(toolPathString);
try {
toolPath = PathFragment.createForOs(toolPathString, execOs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do understand correctly that this will raise an error where there was none, but that's OK because if Bazel encountered C:\WINDOWS32\SYSTEM\ style path on Linux, the situation would be unsalvageable anyway?

Actually: what about the case where Bazel runs on Linux, there is a toolchain definition for Windows somewhere but it's not used? Then it looks like Bazel would raise an error where there were none. I'm not sure if this scenario happens a lot because it would require someone to depend on the toolchain, which they don't if it's just a toolchain that's never selected.

} catch (PathFragment.PathUnsupportedOnThisOs e) {
throw infoError(
toolStruct, "The 'path' field of tool is not a valid path: %s", e.getMessage());
}
if (toolPath.isEmpty()) {
throw infoError(toolStruct, "The 'path' field of tool must be a nonempty string.");
}
Expand Down Expand Up @@ -1723,7 +1731,7 @@ static CcToolchainFeatures.Tool toolFromStarlark(StarlarkInfo toolStruct) throws

/** Creates an {@link ActionConfig} from a {@link StarlarkInfo}. */
@VisibleForTesting
static ActionConfig actionConfigFromStarlark(StarlarkInfo actionConfigStruct)
static ActionConfig actionConfigFromStarlark(StarlarkInfo actionConfigStruct, OS execOs)
throws EvalException {
checkRightProviderType(actionConfigStruct, "action_config");
String actionName =
Expand All @@ -1748,7 +1756,7 @@ static ActionConfig actionConfigFromStarlark(StarlarkInfo actionConfigStruct)
ImmutableList<StarlarkInfo> toolStructs =
getStarlarkProviderListFromStarlarkField(actionConfigStruct, "tools");
for (StarlarkInfo toolStruct : toolStructs) {
toolBuilder.add(toolFromStarlark(toolStruct));
toolBuilder.add(toolFromStarlark(toolStruct, execOs));
}

ImmutableList.Builder<FlagSet> flagSetBuilder = ImmutableList.builder();
Expand Down
17 changes: 6 additions & 11 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,21 +979,16 @@ private static void checkToolPath(PathFragment toolPath, CToolchain.Tool.PathOri

/** Returns the path to this action's tool relative to the provided crosstool path. */
String getToolPathString(PathFragment ccToolchainPath) {
switch (toolPathOrigin) {
case CROSSTOOL_PACKAGE:
return switch (toolPathOrigin) {
case CROSSTOOL_PACKAGE -> {
// Legacy behavior.
if (toolPathString == null) {
toolPathString = ccToolchainPath.getRelative(toolPathFragment).getSafePathString();
}
return toolPathString;

case FILESYSTEM_ROOT: // fallthrough.
case WORKSPACE_ROOT:
return toolPathFragment.getSafePathString();
}

// Unreached.
throw new IllegalStateException();
yield toolPathString;
}
case FILESYSTEM_ROOT, WORKSPACE_ROOT -> toolPathFragment.getSafePathString();
};
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/vfs/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
"//src/main/java/com/google/devtools/build/lib/util:filetype",
"//src/main/java/com/google/devtools/build/lib/util:os",
"//third_party:error_prone_annotations",
"//third_party:guava",
"//third_party:jsr305",
Expand Down
26 changes: 23 additions & 3 deletions src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,23 @@ public interface OsPathPolicy {

boolean isCaseSensitive();

// We *should* use a case-insensitive policy for OS.DARWIN, but we currently don't handle this.
OsPathPolicy HOST_POLICY =
OS.getCurrent() == OS.WINDOWS ? WindowsOsPathPolicy.INSTANCE : UnixOsPathPolicy.INSTANCE;
OsPathPolicy HOST_POLICY = getFilePathOs(OS.getCurrent());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: SERVER_POLICY? ("host" is ambiguous, because it's also the old name for "exec")

For this reason, a comment would also be welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding (per https://bazel.build/extending/platforms) is that we've retroactively redefined "host" to mean "the platform Bazel runs on" and introduced "exec" to mean "the platform where an action executes". The only reason it's ambiguous is that we haven't followed through in replacing "host" with "exec" everywhere it matters (notably, --host_* flags which should really be called --exec_*).

A comment would be welcome, yes, but IMO we should stick to "host" or "exec" instead of introducing yet another term.


static OsPathPolicy getFilePathOs() {
return HOST_POLICY;
}

static OsPathPolicy getFilePathOs(OS os) {
if (os != OS.WINDOWS) {
// We *should* use a case-insensitive policy for OS.DARWIN, but we currently don't handle
// this.
return UnixOsPathPolicy.INSTANCE;
}
return os == OS.getCurrent()
? WindowsOsPathPolicy.INSTANCE
: WindowsOsPathPolicy.CROSS_PLATFORM_INSTANCE;
}

/** Utilities for implementations of {@link OsPathPolicy}. */
class Utils {
/**
Expand Down Expand Up @@ -135,4 +144,15 @@ static int removeRelativePaths(String[] segments, int starti, boolean isAbsolute
return segmentCount;
}
}

/**
* Unchecked exception thrown by {@link OsPathPolicy} implementations when a path cannot be
* normalized on the current host OS.
*/
final class UncheckedPathUnsupportedOnThisOsException
extends UnsupportedOperationException {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be an unchecked exception? It looks like it's only thrown from ShortPathResolver.resolve(), which itself is only called through OsPathPolicy from PathFragment() so it should not be too bad to either use its checked version or turn this into an erroneous return value instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this is also reachable via PathFragment#getRelative, which has ~100 callsites in prod code in Bazel.

But as PathFragment#getRelative can validly throw the exception (e.g. C:\foobar is joined with foo~1.txt on Linux for Windows), this could also result in the unchecked exception being thrown when C++ toolchain logic internally uses this function. I'm not sure what a good solution to this would look like.

UncheckedPathUnsupportedOnThisOsException(String message) {
super(message);
}
}
}
29 changes: 26 additions & 3 deletions src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,28 @@ public abstract class PathFragment

/** Creates a new normalized path fragment. */
public static PathFragment create(String path) {
return createInternal(path, OS);
}

public static PathFragment createForOs(String path, com.google.devtools.build.lib.util.OS os)
throws PathUnsupportedOnThisOs {
try {
return createInternal(path, OsPathPolicy.getFilePathOs(os));
} catch (OsPathPolicy.UncheckedPathUnsupportedOnThisOsException e) {
throw new PathUnsupportedOnThisOs(e);
}
}

private static PathFragment createInternal(String path, OsPathPolicy osPathPolicy) {
if (path.isEmpty()) {
return EMPTY_FRAGMENT;
}
int normalizationLevel = OS.needsToNormalize(path);
int normalizationLevel = osPathPolicy.needsToNormalize(path);
String normalizedPath =
normalizationLevel != OsPathPolicy.NORMALIZED
? OS.normalize(path, normalizationLevel)
? osPathPolicy.normalize(path, normalizationLevel)
: path;
int driveStrLength = OS.getDriveStrLength(normalizedPath);
int driveStrLength = osPathPolicy.getDriveStrLength(normalizedPath);
return makePathFragment(normalizedPath, driveStrLength);
}

Expand Down Expand Up @@ -126,6 +139,16 @@ public boolean isEmpty() {
*/
public abstract int getDriveStrLength();

/**
* Thrown by {@link #createForOs(String, com.google.devtools.build.lib.util.OS)} * when a path
* cannot be normalized on the current host OS.
*/
public static final class PathUnsupportedOnThisOs extends Exception {
private PathUnsupportedOnThisOs(OsPathPolicy.UncheckedPathUnsupportedOnThisOsException e) {
super(e.getMessage(), e);
}
}

private static final class RelativePathFragment extends PathFragment {
// DON'T add any fields here unless you know what you are doing. Adding another field will
// increase the shallow heap of a RelativePathFragment instance beyond the current value of 16
Expand Down
17 changes: 13 additions & 4 deletions src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@
@VisibleForTesting
class WindowsOsPathPolicy implements OsPathPolicy {

static final WindowsOsPathPolicy INSTANCE = new WindowsOsPathPolicy();
static final WindowsOsPathPolicy INSTANCE =
new WindowsOsPathPolicy(new DefaultShortPathResolver());

static final WindowsOsPathPolicy CROSS_PLATFORM_INSTANCE =
new WindowsOsPathPolicy(new CrossPlatformShortPathResolver());

static final int NEEDS_SHORT_PATH_NORMALIZATION = NEEDS_NORMALIZE + 1;

Expand All @@ -33,7 +37,7 @@ class WindowsOsPathPolicy implements OsPathPolicy {
private final ShortPathResolver shortPathResolver;

interface ShortPathResolver {
String resolveShortPath(String path);
String resolveShortPath(String path) throws UncheckedPathUnsupportedOnThisOsException;
}

static class DefaultShortPathResolver implements ShortPathResolver {
Expand All @@ -47,10 +51,15 @@ public String resolveShortPath(String path) {
}
}

WindowsOsPathPolicy() {
this(new DefaultShortPathResolver());
static class CrossPlatformShortPathResolver implements ShortPathResolver {
@Override
public String resolveShortPath(String path) {
throw new UncheckedPathUnsupportedOnThisOsException(
"Windows short paths can only be resolved on a Windows host: " + path);
}
}

@VisibleForTesting
WindowsOsPathPolicy(ShortPathResolver shortPathResolver) {
this.shortPathResolver = shortPathResolver;
}
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ java_test(
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:os",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/net/starlark/java/eval",
Expand Down
Loading
Loading