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

jesseschalken · 2022-08-02T09:30:03Z

This branch

Fixes various compilation and runtime errors when building the C++ Windows launcher using a MinGW toolchain on Linux
Fixes Windows target platform detection to use the target platform instead of the host platform
Removes the execution platform configuration from the $launcher attributes so that the launcher is compiled for the target platform
Adds target platform constraints to the autogenerated Windows toolchains so that they can be selected with target platform constraints as a replacement for --compiler=... when using --incompatible_enable_cc_toolchain_resolution

…d sh_binary Windows launcher The launcher needs to run on the target platform, not the execution platform.

… sh_binary

- Fixes compiling launcher_main.cc with old MinGW which defaults to an old version of C++ without make_unique. - Solves an error with using 10'000'000 syntax on old GCC. - Provides std::filesystem for java_launcher.cc

…lised by bazel::windows::Normalize() anyway

… target platform

fmeum · 2022-08-02T09:53:44Z

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

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


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.

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

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

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.

@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?

@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.

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.

@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")))

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?

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.

sgowroji · 2022-09-01T05:36:37Z

Hello @jesseschalken, Could you please resolve the code conflicts. I have shared it to team, for further review.

…r-on-mingw

jesseschalken · 2022-09-01T17:05:30Z

@sgowroji Thanks. I've merged master but I am yet to fix the Windows test failures (I'm still trying to get them to pass locally on master). Hoping someone beats me to it.

comius

Thanks for your changes! I'm just working on C++ plaformisation and your change probably fixes a couple of more things than expected, judging from the title :)

comius · 2022-09-01T19:07:53Z

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

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


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.

comius · 2022-09-01T19:10:01Z

src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java

@@ -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();


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.

comius · 2022-09-01T19:17:42Z

src/main/java/com/google/devtools/build/lib/util/OsUtils.java

@@ -26,7 +26,11 @@ private OsUtils() {
  }

  public static String executableExtension(OS os) {
-    return os == OS.WINDOWS ? ".exe" : "";
+    return executableExtension(os == OS.WINDOWS);


Please mark this function private. We don't want to encourage people to use it.

comius · 2022-09-01T19:19:24Z

src/main/java/com/google/devtools/build/lib/util/OsUtils.java

/**

Please mark the following function executableExtension() @deprecated. There are some uses left, that should be cleaned up if we want C++ platformization to fully work.

comius · 2022-09-01T19:24:52Z

Tests might be failing because --platforms=windows isn't passed (or some autodetection is failing).

comius · 2023-08-22T05:10:05Z

I'm closing this, becuase there's a lack of response for about a year. Feel free to reopen it, if you want to move it along.

jesseschalken added 10 commits August 2, 2022 18:52

Remove incorrect exec config transition for java_binary, py_binary an…

7b8bb0d

…d sh_binary Windows launcher The launcher needs to run on the target platform, not the execution platform.

Fix Windows detection when cross compiling java_binary, py_binary and…

29be9a1

… sh_binary

Fix Windows java launcher MinGW compatibility

3ad5b34

Make the Windows Java launcher fully statically linked

bfbb0be

Set C++ standard version to C++17 on all launcher targets

7abb6b1

- Fixes compiling launcher_main.cc with old MinGW which defaults to an old version of C++ without make_unique. - Solves an error with using 10'000'000 syntax on old GCC. - Provides std::filesystem for java_launcher.cc

Allow backslashes in input to AsWindowsPathImpl(), they will be norma…

05ebbf5

…lised by bazel::windows::Normalize() anyway

Move compiler constraints on Windows toolchains from exec platform to…

3dfd8f5

… target platform

Add msvc compiler constraint to MSVC toolchains

8f03b44

Fix missing l modifier when formatting wchar_t*

694e0f3

Link launcher binary with -municode on MinGW

55995fe

jesseschalken requested a review from lberki as a code owner August 2, 2022 09:30

fmeum reviewed Aug 2, 2022

View reviewed changes

meteorcloudy requested a review from comius as a code owner August 23, 2022 08:40

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Sep 1, 2022

Merge remote-tracking branch 'origin/master' into fix-windows-launche…

7e567bb

…r-on-mingw

comius requested changes Sep 1, 2022

View reviewed changes

Wyverald requested a review from meteorcloudy September 7, 2022 13:43

meteorcloudy added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 18, 2022

oquenchil assigned comius Jan 20, 2023

comius closed this Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jesseschalken commented Aug 2, 2022 •

edited

Loading

fmeum Aug 2, 2022

jesseschalken Aug 2, 2022

fmeum Aug 2, 2022

fmeum Aug 2, 2022 •

edited

Loading

jesseschalken Aug 13, 2022

Wyverald Sep 1, 2022

comius Sep 1, 2022

jesseschalken Sep 2, 2022

Wyverald Sep 2, 2022

comius Sep 2, 2022 •

edited

Loading

sgowroji commented Sep 1, 2022

jesseschalken commented Sep 1, 2022

comius left a comment •

edited

Loading

comius Sep 1, 2022

comius Sep 1, 2022

comius Sep 1, 2022

comius Sep 1, 2022

comius commented Sep 1, 2022

comius commented Aug 22, 2023

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

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

Conversation

jesseschalken commented Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comius Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

sgowroji commented Sep 1, 2022

jesseschalken commented Sep 1, 2022

comius left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comius commented Sep 1, 2022

comius commented Aug 22, 2023

jesseschalken commented Aug 2, 2022 •

edited

Loading

fmeum Aug 2, 2022 •

edited

Loading

comius Sep 2, 2022 •

edited

Loading

comius left a comment •

edited

Loading