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

Fix Linux to Windows cross compilation of py_binary, java_binary and sh_binary using MinGW #16019

Closed
wants to merge 11 commits into from
Closed

Fix Linux to Windows cross compilation of py_binary, java_binary and sh_binary using MinGW #16019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7b8bb0d
Remove incorrect exec config transition for java_binary, py_binary an…
jesseschalken Jul 28, 2022
29be9a1
Fix Windows detection when cross compiling java_binary, py_binary and…
jesseschalken Jan 27, 2022
3ad5b34
Fix Windows java launcher MinGW compatibility
jesseschalken Feb 2, 2022
bfbb0be
Make the Windows Java launcher fully statically linked
jesseschalken Feb 2, 2022
7abb6b1
Set C++ standard version to C++17 on all launcher targets
jesseschalken Feb 3, 2022
05ebbf5
Allow backslashes in input to AsWindowsPathImpl(), they will be norma…
jesseschalken Feb 3, 2022
3dfd8f5
Move compiler constraints on Windows toolchains from exec platform to…
jesseschalken Feb 6, 2022
8f03b44
Add msvc compiler constraint to MSVC toolchains
jesseschalken Feb 6, 2022
694e0f3
Fix missing l modifier when formatting wchar_t*
jesseschalken Feb 6, 2022
55995fe
Link launcher binary with -municode on MinGW
jesseschalken Mar 14, 2022
7e567bb
Merge remote-tracking branch 'origin/master' into fix-windows-launche…
jesseschalken Sep 1, 2022
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
3 changes: 3 additions & 0 deletions src/main/cpp/util/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
load("//src/tools/launcher:win_rules.bzl", "CC_17_COPT")

# Description:
# C++ utility source for Bazel
package_group(
Expand Down Expand Up @@ -52,6 +54,7 @@ cc_library(
"path.h",
"path_platform.h",
],
copts = CC_17_COPT,
visibility = [
":ijar",
"//src/main/tools:__pkg__",
Expand Down
8 changes: 1 addition & 7 deletions src/main/cpp/util/path_windows.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,14 +276,8 @@ static bool AsWindowsPathImpl(const std::basic_string<char_type>& path,
}

std::basic_string<char_type> mutable_path = path;
if (path[0] == '/') {
if (error) {
*error = "Unix-style paths are unsupported";
}
return false;
}

if (path[0] == '\\') {
if (path[0] == '\\' || path[0] == '/') {
// This is an absolute Windows path on the current drive, e.g. "\foo\bar".
std::basic_string<char_type> drive(1, GetCurrentDrive());
drive.push_back(':');
Expand Down
2 changes: 2 additions & 0 deletions src/main/cpp/util/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,14 @@ char (&ArraySizeHelper(const T (&array)[N]))[N];

#define arraysize(array) (sizeof(ArraySizeHelper(array)))

#ifndef __MINGW32__
#ifdef _WIN32
// TODO(laszlocsomor) 2016-11-28: move pid_t usage out of global_variables.h and
// wherever else it appears. Find some way to not have to declare a pid_t here,
// either by making PID handling platform-independent or some other idea; remove
// the following typedef afterwards.
typedef int pid_t;
#endif // _WIN32
#endif // __MINGW32__

#endif // BAZEL_SRC_MAIN_CPP_UTIL_PORT_H_
1 change: 1 addition & 0 deletions src/main/cpp/util/strings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <memory> // unique_ptr
#include <string>
#include <vector>
#include <sstream>

#ifdef BLAZE_OPENSOURCE
#include <string.h>
Expand Down
21 changes: 21 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 @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.analysis;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.devtools.build.lib.cmdline.RepositoryMapping.ALWAYS_FALLBACK;
import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;

import com.google.common.annotations.VisibleForTesting;
Expand Down Expand Up @@ -53,11 +54,13 @@
import com.google.devtools.build.lib.analysis.config.FragmentCollection;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -1242,6 +1245,24 @@ public ExecGroupCollection getExecGroups() {
return execGroupCollection;
}

private final static ConstraintSettingInfo osConstraintSetting;
private final static ConstraintValueInfo windowsConstraintValue;

static {
try {
Label osConstraintSettingLabel = Label.parseAbsolute("@platforms//os:os", ALWAYS_FALLBACK);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will probably fail with Bzlmod as it doesn't use a repository mapping - with Bzlmod, the platforms module will manifest as a repository named something like @platforms~0.0.4.

You should be able to use:

Label.parseWithRepoContext("@platforms//os:os", repoContext.of(RepositoryName.MAIN, getRule.getPackage().getRepositoryMapping()));

but that won't work from a static context.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Should I just parse the labels directly in isTargetOsWindows?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I unfortunately don't know enough about constraint handling in Java code to answer that question - I would leave it for the reviewer.

Copy link
Collaborator

@fmeum fmeum Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking about this some more, it's more involved and what I suggested above is incorrect: The rule may be instantiated in any repository, not necessarily the main repository. But these repositories may not depend on the platforms module and thus may not even see @platforms in their repository mapping.

@Wyverald can probably help here, but may still be OOO.

Edit: A possible solution could be to introduce aliases for the constraint values in bazel_tools, which AFAIK is always available under that name. This would probably allow you to keep using Label.parseAbsolute, so better keep that code around until someone more knowledgeable suggests a solution.

Copy link
Author

Choose a reason for hiding this comment

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

@Wyverald Do you happen to know the best way to test for the @platforms//os:windows constraint on the target platform inside a Java rule implementation?

Copy link
Member

Choose a reason for hiding this comment

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

@comius would be the best person to answer this, I think. I really don't know whether we're supposed to grab labels like this in Java code.

With that said, what @fmeum pointed out still stands -- we can't rely on the current repository having visibility into the @platforms repo. Depending on the answer to the previous question, we might have ways to work around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move all references to Windows out of RuleContext.

RuleContext should remain a general device, and is no place for specifics. You can either put the code into into each rule or providing a helper class under rules or bazel/rules package.

This will allso make it possible to Starlarkify the rule's your modifying. There's already available API in Starlark ctx.target_platform_has_constraint.

Creating a providers instead of using an implicit dependency to @platforms//os:windows is also not rewritable to Starlark (you can't create a ConstraintInfoProvider). Please use the example you found in copy_file.

Copy link
Author

Choose a reason for hiding this comment

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

@comius Thanks. How do I get a Label for @platforms//os:windows to use in Bazel{Sh,Py,Java}BinaryRule.build? It looks like RuleDefinitionEnvironment can only give Labels for things in @bazel_tools. Will this work?

.add(
    attr("_windows_constraint", LABEL)
        .value(Label.parseAbsoluteUnchecked("@platforms//os:windows")))

Copy link
Member

Choose a reason for hiding this comment

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

the above will likely not work with Bzlmod (same reason as @fmeum described earlier). @bazel_tools is special since it's a "built-in module" which is shipped with the Bazel binary; @platforms is not.

You would be able to use labels from @platforms if you were defining these rules in Starlark (in the builtins_bzl stuff, for example). Not sure where we are on that front -- I think java_binary is already Starlarkified?

Copy link
Contributor

@comius comius Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It think this would work, except if I'm missing some bzlmod machinery that is invoked on Labels in Starlark. It's not different from other implicit dependencies - which also point at various different location outside of Bazel.

There's another problem: it will only work in Bazel, not in a monorepo. We use getToolsRepository to map it to absolute label. Can you extend it with getPlatfromsRepository()? This should make it possible to remap @platforms as well.

Label windowsConstraintValueLabel = Label.parseAbsolute("@platforms//os:windows", ALWAYS_FALLBACK);
osConstraintSetting = ConstraintSettingInfo.create(osConstraintSettingLabel);
windowsConstraintValue = ConstraintValueInfo.create(osConstraintSetting, windowsConstraintValueLabel);
} catch (LabelSyntaxException e) {
throw new RuntimeException(e);
}
}

public boolean isTargetOsWindows() {
return targetPlatformHasConstraint(windowsConstraintValue);
}

public boolean targetPlatformHasConstraint(ConstraintValueInfo constraintValue) {
if (toolchainContexts == null || toolchainContexts.getTargetPlatform() == null) {
return false;
Expand Down
5 changes: 0 additions & 5 deletions src/main/java/com/google/devtools/build/lib/analysis/actions/LauncherFileWriteAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,6 @@ private LauncherFileWriteAction(

@Override
public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
// TODO(laszlocsomor): make this code check for the execution platform, not the host platform,
// once Bazel supports distinguishing between the two.
// OS.getCurrent() returns the host platform, not the execution platform, which is fine in a
// single-machine execution environment, but problematic with remote execution.
Preconditions.checkState(OS.getCurrent() == OS.WINDOWS);
return out -> {
try (InputStream in = ctx.getInputPath(this.launcher).getInputStream()) {
ByteStreams.copy(in, out);
Expand Down
9 changes: 4 additions & 5 deletions src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,25 +37,24 @@ private AnalysisTestActionBuilder() {}
*/
public static void writeAnalysisTestAction(
RuleContext ruleContext, AnalysisTestResultInfo testResultInfo) {
// TODO(laszlocsomor): Use the execution platform, not the host platform.
boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS;
boolean isTargetOsWindows = ruleContext.isTargetOsWindows();
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @brandjon @tetromino
No need to do anything (maybe a TODO) otherwise just a driveby comment.

Do you know why we need special casingAnalysisTestResultInfo for Windows? This should also be general like RuleContext.

String escapedMessage =
isExecutedOnWindows
isTargetOsWindows
? testResultInfo.getMessage().replace("%", "%%")
// Prefix each character with \ (double-escaped; once in the string, once in the
// replacement sequence, which allows backslash-escaping literal "$"). "." is put in
// parentheses because ErrorProne is overly vigorous about objecting to "." as an
// always-matching regex (b/201772278).
: testResultInfo.getMessage().replaceAll("(.)", "\\\\$1");
StringBuilder sb = new StringBuilder();
if (isExecutedOnWindows) {
if (isTargetOsWindows) {
sb.append("@echo off\n");
}
for (String line : Splitter.on("\n").split(escapedMessage)) {
sb.append("echo ").append(line).append("\n");
}
sb.append("exit ");
if (isExecutedOnWindows) {
if (isTargetOsWindows) {
sb.append("/b ");
}
sb.append(testResultInfo.getSuccess() ? "0" : "1");
Expand Down
9 changes: 3 additions & 6 deletions src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,8 @@ private TestParams createTestAction(int shards)
AnalysisEnvironment env = ruleContext.getAnalysisEnvironment();
ArtifactRoot root = ruleContext.getTestLogsDirectory();

// TODO(laszlocsomor), TODO(ulfjack): `isExecutedOnWindows` should use the execution platform,
// not the host platform. Once Bazel can tell apart these platforms, fix the right side of this
// initialization.
final boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS;
final boolean isUsingTestWrapperInsteadOfTestSetupScript = isExecutedOnWindows;
final boolean isTargetOsWindows = ruleContext.isTargetOsWindows();
final boolean isUsingTestWrapperInsteadOfTestSetupScript = isTargetOsWindows;

NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder();
inputsBuilder.addTransitive(
Expand Down Expand Up @@ -448,7 +445,7 @@ private TestParams createTestAction(int shards)
config,
ruleContext.getWorkspaceName(),
(!isUsingTestWrapperInsteadOfTestSetupScript
|| executionSettings.needsShell(isExecutedOnWindows))
|| executionSettings.needsShell(isTargetOsWindows))
? ShToolchain.getPathOrError(ruleContext.getExecutionPlatform())
: null,
cancelConcurrentTests,
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaRuleClasses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,6 @@ <li>If you are using any other launcher, native (C++) dependencies are staticall
.add(attr(":java_launcher", LABEL).value(JavaSemantics.JAVA_LAUNCHER)) // blaze flag
.add(
attr("$launcher", LABEL)
.cfg(ExecutionTransitionFactory.create())
.value(env.getToolsLabel("//tools/launcher:launcher")))
.build();
}
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.shell.ShellUtils.TokenizationException;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.ShellEscaper;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand Down Expand Up @@ -350,7 +349,7 @@ public Artifact createStubAction(
ImmutableList<String> jvmFlagsList = ImmutableList.copyOf(jvmFlags);
arguments.add(Substitution.ofSpaceSeparatedList("%jvm_flags%", jvmFlagsList));

if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
List<String> jvmFlagsForLauncher = jvmFlagsList;
try {
jvmFlagsForLauncher = new ArrayList<>(jvmFlagsList.size());
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.value(env.getToolsLabel("//tools/zip:zipper")))
.add(
attr("$launcher", LABEL)
.cfg(ExecutionTransitionFactory.create())
.value(env.getToolsLabel("//tools/launcher:launcher")))
.build();
}
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyTestRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.override(attr("stamp", TRISTATE).value(TriState.NO))
.add(
attr("$launcher", LABEL)
.cfg(ExecutionTransitionFactory.create())
.value(env.getToolsLabel("//tools/launcher:launcher")))
.add(attr(":lcov_merger", LABEL).value(BaseRuleClasses.getCoverageOutputGeneratorLabel()))
// Add the script as an attribute in order for py_test to output code coverage results for
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ public void createExecutable(
// never used so we skip it.
if (!buildPythonZip) {
Artifact stubOutput =
OS.getCurrent() == OS.WINDOWS
ruleContext.isTargetOsWindows()
? common.getPythonStubArtifactForWindows(executable)
: executable;
createStubFile(ruleContext, stubOutput, common, /* isForZipFile= */ false);
Expand All @@ -235,7 +235,7 @@ public void createExecutable(
Artifact zipFile = common.getPythonZipArtifact(executable);

// TODO(b/234923262): Take exec_group into consideration when selecting sh tools
if (OS.getCurrent() != OS.WINDOWS) {
if (!ruleContext.isTargetOsWindows()) {
PathFragment shExecutable = ShToolchain.getPathForHost(ruleContext.getConfiguration());
String pythonExecutableName = "python3";
// NOTE: keep the following line intact to support nix builds
Expand All @@ -259,7 +259,7 @@ public void createExecutable(
}

// On Windows, create the launcher.
if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
createWindowsExeLauncher(
ruleContext,
// In the case where the second-stage interpreter is in runfiles, the launcher is passed
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShBinaryRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
return builder
.add(
attr("$launcher", LABEL)
.cfg(ExecutionTransitionFactory.create())
.value(environment.getToolsLabel("//tools/launcher:launcher")))
.build();
}
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShTestRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
.add(attr(":lcov_merger", LABEL).value(BaseRuleClasses.getCoverageOutputGeneratorLabel()))
.add(
attr("$launcher", LABEL)
.cfg(ExecutionTransitionFactory.create())
.value(environment.getToolsLabel("//tools/launcher:launcher")))
// Add the script as an attribute in order for sh_test to output code coverage results for
// code covered by CC binaries invocations.
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.PathFragment;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -70,7 +69,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
ruleContext.getConfiguration().legacyExternalRunfiles());

Artifact mainExecutable =
(OS.getCurrent() == OS.WINDOWS) ? launcherForWindows(ruleContext, symlink, src) : symlink;
ruleContext.isTargetOsWindows() ? launcherForWindows(ruleContext, symlink, src) : symlink;
if (!symlink.equals(mainExecutable)) {
filesToBuildBuilder.add(mainExecutable);
runfilesBuilder.addArtifact(symlink);
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
import com.google.devtools.build.lib.rules.java.OneVersionCheckActionBuilder;
import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -217,7 +216,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
JavaCompilationArtifacts.Builder javaArtifactsBuilder = new JavaCompilationArtifacts.Builder();

Artifact executable; // the artifact for the rule itself
if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
executable =
ruleContext.getImplicitOutputArtifact(ruleContext.getTarget().getName() + ".exe");
} else {
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.OnDemandString;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -94,8 +93,7 @@ enum CommandType {
private static Pair<CommandType, String> determineCommandTypeAndAttribute(
RuleContext ruleContext) {
AttributeMap attributeMap = ruleContext.attributes();
// TODO(pcloudy): This should match the execution platform instead of using OS.getCurrent()
if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) {
return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps");
}
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@
import com.google.devtools.build.lib.rules.java.JavaConfiguration.OneVersionEnforcementLevel;
import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider.JavaOutput;
import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.StringCanonicalizer;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand Down Expand Up @@ -217,7 +216,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
if (createExecutable) {
// This artifact is named as the rule itself, e.g. //foo:bar_bin -> bazel-bin/foo/bar_bin
// On Windows, it's going to be bazel-bin/foo/bar_bin.exe
if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
executableForRunfiles =
ruleContext.getImplicitOutputArtifact(ruleContext.getTarget().getName() + ".exe");
} else {
Expand Down
18 changes: 10 additions & 8 deletions src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@

/** Implementation for the {@code java_runtime} rule. */
public class JavaRuntime implements RuleConfiguredTargetFactory {
// TODO(lberki): This is incorrect but that what the Jvm configuration fragment did. We'd have the
// the ability to do better if we knew what OS the BuildConfigurationValue refers to.
private static final String BIN_JAVA = "bin/java" + OsUtils.executableExtension();
private static String getBinJava(boolean isWindows) {
return "bin/java" + OsUtils.executableExtension(isWindows);
}

@Override
@Nullable
Expand All @@ -67,9 +67,11 @@ public ConfiguredTarget create(RuleContext ruleContext)
javaHome = javaHome.getRelative(javaHomeAttribute);
}

PathFragment javaBinaryExecPath = javaHome.getRelative(BIN_JAVA);
boolean isWindows = ruleContext.isTargetOsWindows();

PathFragment javaBinaryExecPath = javaHome.getRelative(getBinJava(isWindows));
PathFragment javaBinaryRunfilesPath =
getRunfilesJavaExecutable(javaHome, ruleContext.getLabel());
getRunfilesJavaExecutable(javaHome, ruleContext.getLabel(), isWindows);

Artifact java = ruleContext.getPrerequisiteArtifact("java");
if (java != null) {
Expand Down Expand Up @@ -149,14 +151,14 @@ static PathFragment defaultJavaHome(Label javabase, boolean siblingRepositoryLay
return javabase.getPackageIdentifier().getExecPath(siblingRepositoryLayout);
}

private static PathFragment getRunfilesJavaExecutable(PathFragment javaHome, Label javabase) {
private static PathFragment getRunfilesJavaExecutable(PathFragment javaHome, Label javabase, boolean isWindows) {
if (javaHome.isAbsolute() || javabase.getRepository().isMain()) {
return javaHome.getRelative(BIN_JAVA);
return javaHome.getRelative(getBinJava(isWindows));
} else {
return javabase
.getRepository()
.getRunfilesPath()
.getRelative(BIN_JAVA);
.getRelative(getBinJava(isWindows));
}
}
}
7 changes: 3 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ public PyRuntimeInfo getRuntimeFromToolchain() {
public void initBinary(List<Artifact> srcs) {
Preconditions.checkNotNull(version);

if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
executable =
ruleContext.getImplicitOutputArtifact(ruleContext.getTarget().getName() + ".exe");
} else {
Expand All @@ -662,8 +662,7 @@ public void initBinary(List<Artifact> srcs) {

if (ruleContext.getFragment(PythonConfiguration.class).buildPythonZip()) {
filesToBuildBuilder.add(getPythonZipArtifact(executable));
} else if (OS.getCurrent() == OS.WINDOWS) {
// TODO(bazel-team): Here we should check target platform instead of using OS.getCurrent().
} else if (ruleContext.isTargetOsWindows()) {
// On Windows, add the python stub launcher in the set of files to build.
filesToBuildBuilder.add(getPythonStubArtifactForWindows(executable));
}
Expand All @@ -689,7 +688,7 @@ private Artifact getArtifactWithExtension(Artifact executable, String extension)
executable.getOutputDirRelativePath(
ruleContext.getConfiguration().isSiblingRepositoryLayout());
String fileName = executable.getFilename();
if (OS.getCurrent() == OS.WINDOWS) {
if (ruleContext.isTargetOsWindows()) {
Preconditions.checkArgument(fileName.endsWith(".exe"));
fileName = fileName.substring(0, fileName.length() - 4) + extension;
} else {
Expand Down
Loading
Loading