Support multiple execution platforms with system_cxx_toolchain #629

Overhatted · 2024-04-28T21:02:19Z

As mentioned in #612, I have a C++ project that can be built for either Linux or Windows, without any cross-compilation so Linux and Windows are also the two execution platforms. Since there is remote execution, I wanted to be able to build for Linux from my Windows machine using RE, and vice versa.

From the response I got in the above issue, I need to add an exec_dep to the toolchain, pointing a compiler target, that then has a target_compatible_with with the execution platform the compiler can run on.

I tried to add the target_compatible_with directly to the toolchain but it didn't work, probably intentional.

So I moved some parts of the current system_cxx_toolchain to a separate NativeCompiler provider that is then returned by some rules defining a compiler. This compiler target then has the target_compatible_with attribute.

Can this be merged or should I just have this toolchain in my own repository? It seems useful for everyone, the only disadvantage I can see being that it is not fully backwards compatible. Hopefully I didn't completely misunderstand how execution platforms are supposed to be used.

I improved the Visual Studio example to use this new updated toolchain to easily support remote execution.

Manual testing

I used the Visual Studio example and setup my RE workers with Buildbarn. Unfortunately Buildbarn's Windows workers don't currently work with the linking step, I haven't investigated why. The build targeting Windows made from a Windows host works because the linking is done locally but the one done from a Linux host fails.

Local builds

Windows

buck2 build --target-platforms //buck2_utils/platforms:windows_debug :main

Passes

Linux

buck2 build --target-platforms //buck2_utils/platforms:linux_debug :main

Passes

Remote builds

From Windows

Target Windows

buck2 build --no-remote-cache -c buck2_re_client.enabled=true --target-platforms //buck2_utils/platforms:windows_debug :main

Passes

Target Linux

buck2 build --no-remote-cache -c buck2_re_client.enabled=true --target-platforms //buck2_utils/platforms:linux_debug :main

Passes

From Linux

Target Windows

buck2 build --no-remote-cache -c buck2_re_client.enabled=true --target-platforms //buck2_utils/platforms:windows_debug :main

Linking fails (The system cannot open the device or file specified.)

Target Linux

buck2 build --no-remote-cache -c buck2_re_client.enabled=true --target-platforms //buck2_utils/platforms:linux_debug :main

Passes

Overhatted · 2024-05-23T08:23:05Z

Hello @KapJI, can someone take a look at this PR?

KapJI · 2024-05-23T12:29:21Z

examples/visual_studio/buck2_utils/install.ps1

+		New-Item -ItemType File -Path $TargetPath -Force | Out-Null
+		Copy-Item $SourcePath -Destination $TargetPath | Out-Null


Why 2 steps instead of 1?

Because Copy-Item doesn't create the target directory if it doesn't exist: https://stackoverflow.com/questions/7523497/should-copy-item-create-the-destination-directory-structure
For example the first time the install script is ran, the target "$InstallDirectory" does not exist which makes Copy-Item fail when it tries to copy "main.exe" to "install/main.exe".

KapJI · 2024-05-23T13:00:19Z

prelude/toolchains/cxx/clang/tools.bzl

+path_clang_tools = rule(
+    impl = _path_clang_tools_impl,
+    attrs = {
+        "os": attrs.string(default = _get_current_os()),


This means when you want to target Linux from Windows, it's still be "windows" here. You probably want to have something like _exec_os_type in prelude.

Thanks for the tip. I had't realised that the default could also be a select. But here, since the system_cxx_toolchain doesn't do cross-compilation, we want the "os" to be the target OS so I changed this to buck.target_os_type_arg.

KapJI · 2024-05-23T13:02:50Z

examples/visual_studio/.buckconfig

@@ -11,7 +11,14 @@ fbsource = none
 buck = none

 [parser]
-target_platform_detector_spec = target:root//...->toolchains//:default
+target_platform_detector_spec = target:root//...->root//buck2_utils/platforms:windows_debug


Why change this?

Previously the visual_studio example was using a single target platform (toolchains//:default) that would change one of the constraint_values (debug/release) depending on a read_config call. In this PR I changed the example by creating 4 different target platforms (windows/linux and debug/release) in root//buck2_utils/platforms/BUCK. I only changed this because it seemed like the intended way to use Buck2. Now the target platform is specified by using the "--target-platforms" command line option.

KapJI · 2024-05-23T13:03:47Z

examples/visual_studio/buck2_utils/platforms/BUCK

@@ -0,0 +1,38 @@
+load(":defs.bzl", "execution_platform")
+
+execution_platform(


To make it less confusing.

Suggested change

execution_platform(

execution_platforms(

I did this change manually because I also needed to make the change in buck2_utils/platforms/defs.bzl.

KapJI · 2024-05-23T13:15:19Z

prelude/toolchains/cxx.bzl

-load("@prelude//toolchains/msvc:tools.bzl", "VisualStudio")
-load("@prelude//utils:cmd_script.bzl", "ScriptOs", "cmd_script")
+
+NativeCompiler = provider(


"Compiler" usually means single tool, maybe something like "SystemCxxToolchainInfo"?

KapJI · 2024-05-23T13:27:27Z

prelude/toolchains/cxx.bzl

+def _run_info(args):
+    return None if args == None else RunInfo(args = [args])
+
+def _get_default_compiler() -> str:


Should this be selectified?

I selectified this.

KapJI · 2024-05-23T13:29:11Z

prelude/toolchains/cxx.bzl

        "c_flags": attrs.list(attrs.string(), default = []),
-        "compiler": attrs.string(default = "cl.exe" if host_info().os.is_windows else "clang"),


This is a breaking change for anyone who uses these attributes in system_cxx_toolchain. I don't think we should remove them.

I reintroduced the attributes and I think the end result is fully backwards compatible. I did some tests on Linux and Windows using those parameters.

…sing remote execution

In an earlier version of Buck2, read_config would only work if the cell of the BUCK file had a .buckconfig. That behavior has now changed so the empty .buckconfig file is no longer needed.

link_binaries_locally=True and archive_objects_locally=True were already there. It's probably better to enable local only for all.

Overhatted · 2024-06-05T20:33:23Z

After the review, I made all the changes to this PR in the last 7 commits where the first 6 were done to fix issues pointed out in the review and the last one was done as another change.

Overhatted · 2024-06-07T18:55:08Z

@KapJI Not sure if I'm supposed to ping.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 28, 2024

Overhatted changed the title ~~Multiple target platforms~~ Support multiple execution platforms with system_cxx_toolchain Apr 30, 2024

Overhatted marked this pull request as ready for review April 30, 2024 21:11

Overhatted force-pushed the multiple_target_platforms branch 2 times, most recently from 065fe34 to 5e3b181 Compare May 22, 2024 19:47

KapJI requested changes May 23, 2024

View reviewed changes

Overhatted added 7 commits May 25, 2024 06:15

Added C source files to compile_commands.json

27a221a

Fixed install.ps1 script to work even if the directory doesn't exist

d167511

Added support for specifying the compiler in the system_cxx_toolchain

0c1b452

Updated visual studio example to support cross platform compilation u…

a970f98

…sing remote execution

Added some calls to clang's BUCK to pass the pipeline job

bee4c24

Renamed compiler to SystemCxxToolchainInfo

330e571

Renamed execution_platform to execution_platforms

a379dde

Overhatted force-pushed the multiple_target_platforms branch 3 times, most recently from f97fbe4 to f1e1010 Compare June 4, 2024 21:21

Overhatted added 4 commits June 5, 2024 06:49

Replaced functions with selects

b28d493

Renamed compiler to toolchain_info in system_cxx_toolchain

f07bed7

Readded previously removed system_cxx_toolchain parameters

d000ff8

Removed .buckconfig in Visual Studio's example.

a3e60dc

In an earlier version of Buck2, read_config would only work if the cell of the BUCK file had a .buckconfig. That behavior has now changed so the empty .buckconfig file is no longer needed.

Overhatted force-pushed the multiple_target_platforms branch from f1e1010 to a3e60dc Compare June 5, 2024 05:50

Added link_libraries_locally=True to system_cxx_toolchain.

31482d0

link_binaries_locally=True and archive_objects_locally=True were already there. It's probably better to enable local only for all.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple execution platforms with system_cxx_toolchain #629

Support multiple execution platforms with system_cxx_toolchain #629

Overhatted commented Apr 28, 2024 •

edited

Overhatted commented May 23, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

KapJI May 23, 2024

Overhatted Jun 5, 2024

Overhatted commented Jun 5, 2024

Overhatted commented Jun 7, 2024

		New-Item -ItemType File -Path $TargetPath -Force \| Out-Null
		Copy-Item $SourcePath -Destination $TargetPath \| Out-Null

		@@ -0,0 +1,38 @@
		load(":defs.bzl", "execution_platform")

		execution_platform(

		"c_flags": attrs.list(attrs.string(), default = []),
		"compiler": attrs.string(default = "cl.exe" if host_info().os.is_windows else "clang"),

Support multiple execution platforms with system_cxx_toolchain #629

Are you sure you want to change the base?

Support multiple execution platforms with system_cxx_toolchain #629

Conversation

Overhatted commented Apr 28, 2024 • edited

Manual testing

Local builds

Windows

Linux

Remote builds

From Windows

Target Windows

Target Linux

From Linux

Target Windows

Target Linux

Overhatted commented May 23, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Overhatted commented Jun 5, 2024

Overhatted commented Jun 7, 2024

Overhatted commented Apr 28, 2024 •

edited