Conan integration to import third-party C/C++ dependencies #58

aherrmann · 2022-12-21T17:34:09Z

This PR adds support for importing third-party C/C++ dependencies provided by the Conan package manager.

Detailed motivation and design are contained in the module doctring of the main toolchain module under prelude/toolchains/conan/defs.bzl. In brief, some C/C++ third-party dependencies are difficult to import into a Buck2 project, e.g. the current self-hosted build of Buck2 vendors a binary openssl and skips jemalloc for that reason.

The approach proposed here is to instead use the existing package build definitions for the Conan package manager and build these third-party dependencies with Conan. This approach is similar to how rules_haskell integrates with Stack and Cabal to import third-party Haskell dependencies, or how JVM packages are imported with the help of Coursier in Bazel.

Conan was chosen for its relatively large community package index, compatibility with Linux, MacOS, and Windows, configurability and extensibility, and its support for cross-compilation. Alternatively, vcpkg was considered. However, its choice of CMake as implementation and configuration language makes it harder to integrate. Other interesting comparisons can be found here and here.

This integration aims to leave Buck2 in charge of the overall build, when which targets are built, and which configurations and toolchains are used.

Code map:

prelude/toolchains/conan/defs.bzl the main Starlark module for the Conan integration. Defines the relevant rules.
prelude/toolchains/conan/*.py Python scripts implementing the various build actions.
prelude/toolchains/conan/buckler/ a Conan extension that defines a generator to generate Buck2 import targets for Conan built packages.
examples/prelude/toolchains/BUILD an example Conan toolchain configuration.
examples/prelude/cpp/conan/ an example Conan use-case.

This PR adds tests of the Conan example to the Linux and MacOS CI pipelines. Windows support will be added separately.

Remaining work for future PRs

Better cross-platform support.
Use Conan lock-file bundles to define cross-platform import targets.
Windows support in particular.
Currently builds fail on long-path issues, Conan provides workarounds for these issues that need to be investigated.
Import Buck2’s third-party C/C++ dependencies with Conan.
Cross-compilation support.
Conan has builtin cross-compilation support that needs to be made use of.
Test some more complex packages.
Builds of some more complex packages failed when attempted with a zig cc toolchain, e.g. libmysqlclient fails to find libresolv.so. Investigate these issues. An example setup with the Zig toolchain can be found here.

arlyon · 2022-12-21T20:17:17Z

Excellent, looking forward to reading through this :)

prelude/toolchains/conan/buckler/conanfile.py

arlyon · 2022-12-21T21:31:06Z

prelude/toolchains/conan/defs.bzl

+            static_lib = None
+        native.prebuilt_cxx_library(
+            name = name,
+            deps = deps,  # TODO[AH] Do we need exported_deps?


is exported_deps not needed for transitive deps to work correctly? @ndmitchell please confirm

For c++, exported_deps is required when your dependents need to include your dependency's headers. This could be because your headers that your dependencies include then further directly include ones from your dependency's or if you just otherwise expect your dependents to do so (like if your interfaces are going to require things from your dependency's). Often this isn't required.

Now, when looking at doing something like we're doing here, we're kinda limited by only being able to understand what the underlying specification encodes (and how its other interpretations behave). If the specification doesn't distinguish between public and private dependencies, it is almost certainly treating them all as public (it could be treating them as private, but then everything we sort of need to individually specify their transitive deps and that is rather gross). In that case, I'd recommend encoding them as public deps (i.e. exported_deps) in buck2 as well.

@cjhopman Thanks for the context! I've changed the code to set exported_deps instead.

arlyon · 2022-12-21T21:33:38Z

prelude/toolchains/conan/defs.bzl

+    _conan_cxx_libraries(
+        name = name,
+        main = ":_bundle_" + name,
+        components = components,


I love we support this. Some context about components for others: https://docs.conan.io/en/latest/creating_packages/package_information.html#using-components

arlyon · 2022-12-21T21:46:13Z

prelude/toolchains/conan/defs.bzl

+    attrs = {
+        "arch": attrs.string(doc = "The target architecture"),
+        "os": attrs.string(doc = "The target operating system"),
+        "build_type": attrs.string(doc = "The Conan build-type, e.g. Release or Debug"),


arlyon · 2022-12-21T21:54:32Z

prelude/toolchains/conan/conan_common.py

+    #env["CONAN_REVISIONS_ENABLED"] = "1"
+
+    # Prevent over-allocation.
+    env["CONAN_CPU_COUNT"] = "1"


I take it this means conan build libraries single-threaded. Edit: make is still invoked w/ -j16 on my machine (16 threads)

I take it this means conan build libraries single-threaded.

The intention is to avoid over-allocation due to both Buck2 and Conan parallizing their builds.

Edit: make is still invoked w/ -j16 on my machine (16 threads)

Thanks for testing! IIUC Conan recipes still need to actively use this setting:

Conan recipes can use the cpu_count() tool to build the library using more than one core.

Which package were you building there?

The intention is to avoid over-allocation due to both Buck2 and Conan parallizing their builds

There is a weight on actions that you could try tweaking here. It can basically be thought of as telling buck that the action requires that many cores. it's also been used to limit parallelism due to memory requirements, not cores, but its modeled with just the one limit (trivia note: that's actually different than buck1 which had ability to configure several different weights for different resources)

@cjhopman Thanks! That looks like a useful feature to address this. Given that seemingly not all Conan recipes make use of the CONAN_CPU_COUNT knob, and that the load a parallelized package build will also depend on the size of the package, this does indeed seem like something that will require a bit of tuning. In light of that I'd prefer to leave this to a follow-up PR. I've added a TODO comment to the code to keep track of it.

arlyon · 2022-12-21T21:59:13Z

examples/prelude/cpp/conan/BUILD

+    lockfile = ":lock",
+)
+
+# TODO[AH] Prevent double build of packages.


This is interesting. I take it this cost only appears when actually generating the lockfile itself? That seems like an ok trade-off for now.

Correct, well, when generating the targets file, i.e. when invoking conan install --generator .... But, yes, that's the idea.

arlyon · 2022-12-21T22:02:06Z

prelude/toolchains/conan/conan_init.py

+            user_home=user_home,
+            trace_log=trace_log)
+
+    # TODO[AH] Allow users to define additional remotes.


A conan_remote rule would be cool. You could pass it in similar to the profile similar to deps.

Yes, a conan_remote rule sounds like a good way to do this.

arlyon · 2022-12-21T22:33:16Z

It would be cool to consider this option as well: https://docs.conan.io/en/1.46/reference/conanfile/tools/system/package_manager.html#configuration-properties-that-affect-how-system-package-managers-are-invoked

arlyon · 2022-12-21T22:42:47Z

This is great. I would love to see the package component limitation lifted though it seems like simply a matter of implementing it.

https://github.com/facebookincubator/buck2/blob/76c18df1fe5a0ef8e611779eb080bdcc606b1595/prelude/toolchains/conan/defs.bzl#L344

Otherwise, the code is succinct and clear. The python scripts seem solid, and I've managed to run :update for boost and glm, glfw, and glad and the output file looks correct and includes transitive deps. It even told me I had a missing system package which is cool. It's great that components are included (and I love the syntax) and dependencies are generated though I couldn't test the transitive deps due to the aforementioned issue.

I will let Neil dispense buck2 related wisdom but I am excited that this is ready to land. Hope you have a lovely holiday.

aherrmann · 2022-12-22T13:30:44Z

[...] I've managed to run :update for boost and glm, glfw, and glad [...] though I couldn't test the transitive deps due to the aforementioned issue.

Thanks for testing!

This is great. I would love to see the package component limitation lifted though it seems like simply a matter of implementing it.

I looked into these on Conan central and tried to build boost. It looks like the issue is that boost contains some header-only components for which len(libs) == 0. I've added a case to handle this and update the PR. With that I was able to build boost locally.

For the multiple library components case, i.e. len(libs) > 1, I haven't found an example yet to test this on, so haven't attempted implementing it, yet. Are you aware of such a Conan package? I looked into ffmpeg, which seems to have been one of the packages motivating components, but even that seems to use single library components by now as far as I can tell.

ndmitchell

Looks good - just a few notes/questions.

ndmitchell · 2023-01-04T15:53:50Z

shim/toolchain.bzl

@@ -68,7 +68,7 @@ def _cxx_toolchain(ctx):
                nm = RunInfo(args = ["nm"]),
                # objcopy = ctx.attrs.objcopy_for_shared_library_interface[RunInfo],
                # ranlib = ctx.attrs.ranlib[RunInfo],
-                ranlib = RunInfo(args = ["raninfo"]),
+                ranlib = RunInfo(args = ["ranlib"]),


We no longer use shim/toolchain, since we now use the system toolchains in the examples

ndmitchell · 2023-01-04T15:54:51Z

prelude/toolchains/cxx/zig/defs.bzl

@@ -295,7 +295,7 @@ def _cxx_zig_toolchain_impl(ctx: "context") -> ["provider"]:
    return [ctx.attrs.distribution[DefaultInfo]] + cxx_toolchain_infos(
        platform_name = dist.arch,
        c_compiler_info = CCompilerInfo(
-            compiler = RunInfo(args = cmd_args([zig, "cc"])),
+            compiler = RunInfo(args = cmd_args([zig, "cc"], delimiter = " ")),


Why is this necessary?

The Conan toolchain configures Conan to use the C/C++ toolchain provided by Buck2. It does so by setting the relevant variables in the Conan profile. Some build tools assume that these contain a single path, instead of commands with arguments, so the toolchain generates wrapper scripts that execute the required command. Without this change the command would be written on multiple lines despite the explicit delimiter = " " at a higher level. Perhaps there is a better way to do this? As the comment there indicates I also tried setting quote = "shell", but that had other unwanted effects.

ndmitchell · 2023-01-04T15:55:00Z

prelude/toolchains/cxx.bzl

@@ -41,12 +41,12 @@ def _system_cxx_toolchain_impl(ctx):
            linker_info = LinkerInfo(
                linker = RunInfo(args = ["clang++"]),
                linker_flags = ["-fuse-ld=lld"],
-                archiver = RunInfo(args = ["ar", "rcs"]),
+                archiver = RunInfo(args = ["ar"]),


Why is this necessary?

Defining "rcs" in the archiver's RunInfo breaks with Conan when building a package from source. The problem is that we configure ar rcs as the archiver programm and the build system of the Conan package again passes similar flags, e.g. cs, to the archiver, resulting in a command line of the form ar rcs cs .... This fails with an error of the form:

ar: libz.a: No such file or directory

As explained in madler/zlib#588 (comment) .

If we set ar alone as the archiver then this problem goes away. However, then we need to make sure that Buck2's archive actions pass the correct arguments to ar, which is why this sets use_archiver_flags = True, see here.

Perhaps the core issue is that the archiver field conflates the archiver binary and the flags to be passed to the archiver in an action to create a static archive. If instead it followed the pattern of e.g. CCompilerInfo and there were separate archiver and archiver_flags fields then we could extract the archiver even in a configuration that explicitly configures archiver_flags and sets use_archiver_flags = False.

See also related commit message.

ndmitchell · 2023-01-04T15:55:09Z

prelude/toolchains/cxx.bzl

                archiver_type = archiver_type,
                type = linker_type,
                link_binaries_locally = True,
                archive_objects_locally = True,
-                use_archiver_flags = False,
+                use_archiver_flags = True,


Why is this necessary?

See response to https://github.com/facebookincubator/buck2/pull/58/files#r1061631371.

ndmitchell · 2023-01-04T15:55:57Z

.circleci/config.yml

@@ -209,6 +212,8 @@ jobs:
          command: |
            cd examples/prelude
            cmd.exe /c mklink /D prelude C:\Users\circleci\project\prelude
+            # Skip Conan tests on Windows. TODO[AH] Implement Windows support.
+            Remove-Item -Recurse -Force cpp/conan


We usually do this with host_info if's in the prelude - see where we do it in other places.

The difficulty here is that some of those targets are generated, namely those in examples/prelude/cpp/conan/import/BUILD. See also the internal discussion we had on this from 21/12/2022. Removing the folder seemed like the best option at the time. But, I'm happy to do something else if there's a better option.

ndmitchell · 2023-01-04T15:57:15Z

prelude/toolchains/conan/TARGETS

@@ -0,0 +1,57 @@
+native.export_file(


This file should be named TARGETS.v2, since otherwise internal stuff gets confused

It now uses `buck2 root` to discover the repository root.

This will make it easier to add the lock-file parsing and target generation to it.

To test that the headers are exposed and the library is linked properly.

Defining `"rcs"` in the `archiver`'s `RunInfo` breaks with Conan on MacOS where a prebuilt package is not available for download but has to be built. The problem is that we configure `ar rcs` as the archiver and the Conan package's build system again passes similar flags, e.g. `cs`. The resulting error is as follows: ``` ar: libz.a: No such file or directory ``` As explained in madler/zlib#588 (comment) .

The profile entry in it contains absolute paths to the build tree, which is only valid locally. This requires a workaround.

A toolchain is for the use-case of Conan exposing its configuration and compiler toolchain to another build tool. We have the opposite use-case, and use Conan profiles to that end instead.

The returned second value only contains inputs implied by macros, i.e. `$(location ...)` or similar. Inputs due to the written content must be tracked separately. At least for, in future the produced output artifact may track these inputs automatically.

aherrmann requested review from ndmitchell and arlyon December 21, 2022 17:34

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 Dec 21, 2022

aherrmann force-pushed the conan branch from 97ba694 to 76c18df Compare December 21, 2022 17:38

arlyon reviewed Dec 21, 2022

View reviewed changes

prelude/toolchains/conan/buckler/conanfile.py Outdated Show resolved Hide resolved

arlyon reviewed Dec 21, 2022

View reviewed changes

prelude/toolchains/conan/buckler/conanfile.py Outdated Show resolved Hide resolved

arlyon reviewed Dec 21, 2022

View reviewed changes

ndmitchell reviewed Jan 4, 2023

View reviewed changes

aherrmann added 13 commits February 1, 2023 14:14

Define basic Conan system toolchain

ff44164

Implement conan_lock_update rule

da1746f

make the lock file generator PWD agnostic

80883a7

It now uses `buck2 root` to discover the repository root.

Add docs to the conan toolchain

6ec9bad

Move Conan update into Python script

7042d12

This will make it easier to add the lock-file parsing and target generation to it.

enable Conan revisions

783425b

Parse the Conan lock file

530bb03

Generate Conan package targets

8275929

fix conan defs.bzl import

4fa2e40

conan_package skaffolding

acea26c

typo

e49c173

Output conan_package runner

b5732be

typo

9461e4f

aherrmann added 29 commits February 1, 2023 14:14

Configure the Conan toolchain

6e145d3

Import Conan packages

d7b1fdb

Depend on Conan provided zlib

a09706f

Test zlib using adler32

52c7fb4

To test that the headers are exposed and the library is linked properly.

Enable Conan test on Windows.

8c7737c

Install Conan on CI

f6ce625

Install mingw and cmake on Windows CI

0e5b09c

Configure Conan profile for different OS

ae380a0

Try Conan lock and update on CI

81c5efd

Print generated Conan imports

ede9c14

Remove Conan lock file

0954e0b

The profile entry in it contains absolute paths to the build tree, which is only valid locally. This requires a workaround.

Add a module docstring for Conan

22e7c7c

update conan_lock description

87982a6

Add doc-strings to Conan rules

5e08f1b

Skip Conan on Windows CI

9b34708

Skip Conan Windows deps for now

1a53441

Remove debug lock&import print

91f3d48

todo note

be1f117

Buckler only provides a generator

90f9aac

A toolchain is for the use-case of Conan exposing its configuration and compiler toolchain to another build tool. We have the opposite use-case, and use Conan profiles to that end instead.

todo note

36c0d8d

Fixup rebase issue

dda95b2

Use !r conversions in Python formatting

53de3bc

Remove redundant getattr

68e6834

Handle header only libraries

5e6e951

Set exported_deps

a4abd0f

todo note about CONAN_CPU_COUNT and Buck2 weight

d6b148e

Rename to TARGETS.v2

1e98452

aherrmann force-pushed the conan branch from 275f78e to 1e98452 Compare February 1, 2023 13:56

Jun	JUL	Aug
	06
2022	2023	2024

Conan integration to import third-party C/C++ dependencies #58

Conan integration to import third-party C/C++ dependencies #58

aherrmann commented Dec 21, 2022

arlyon commented Dec 21, 2022

arlyon Dec 21, 2022

cjhopman Jan 27, 2023

aherrmann Feb 1, 2023

arlyon Dec 21, 2022

arlyon Dec 21, 2022

arlyon Dec 21, 2022 •

edited

aherrmann Dec 22, 2022

cjhopman Jan 27, 2023

aherrmann Feb 1, 2023

arlyon Dec 21, 2022

aherrmann Dec 22, 2022

arlyon Dec 21, 2022

aherrmann Dec 22, 2022

arlyon commented Dec 21, 2022

arlyon commented Dec 21, 2022 •

edited

aherrmann commented Dec 22, 2022 •

edited

ndmitchell left a comment

ndmitchell Jan 4, 2023

ndmitchell Jan 4, 2023

aherrmann Feb 1, 2023

ndmitchell Jan 4, 2023

aherrmann Feb 1, 2023

ndmitchell Jan 4, 2023

aherrmann Feb 1, 2023

ndmitchell Jan 4, 2023

aherrmann Feb 1, 2023

ndmitchell Jan 4, 2023

aherrmann Feb 1, 2023

Conan integration to import third-party C/C++ dependencies #58

Are you sure you want to change the base?

Conan integration to import third-party C/C++ dependencies #58

Conversation

aherrmann commented Dec 21, 2022

arlyon commented Dec 21, 2022

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

arlyon Dec 21, 2022 • edited

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

arlyon commented Dec 21, 2022

arlyon commented Dec 21, 2022 • edited

aherrmann commented Dec 22, 2022 • edited

ndmitchell left a 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

arlyon Dec 21, 2022 •

edited

arlyon commented Dec 21, 2022 •

edited

aherrmann commented Dec 22, 2022 •

edited