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.

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

Read flags from pkg-config files #203

Closed
abergmeier opened this issue May 17, 2018 · 10 comments
Closed

Read flags from pkg-config files #203

abergmeier opened this issue May 17, 2018 · 10 comments

Comments

@abergmeier
Copy link

The following does not work with rules_go 0.12 and bazel 0.13:

go_repository(
    name = "foo",
    importpath = "github.com/keroserene/go-webrtc",
    commit = "a1272c08ab1d5ca154c6794ddc5f73d2e576fe1b",
)

First off - I am quite impressed that it works as far as it does.
It seems to not find _cgo_export.h, though.

@abergmeier
Copy link
Author

Using the following it does at least compile:

load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
    name = "go_default_library",
    srcs = [
        "configuration.go",
        "ctestenums.cc",
        "ctestenums.h",
        "datachannel.cc",
        "datachannel.go",
        "datachannel.h",
        "datachannel.hpp",
        "ice.go",
        "logging.go",
        "peerconnection.cc",
        "peerconnection.go",
        "peerconnection.h",
        "sdp.go",
        "utils.go",
    ] + glob(["include/**/*.h"]),
    cgo = True,
    clinkopts = ["-L/lib"],
    cxxopts = [
        "-std=c++0x",
        "-Iexternal/com_github_keroserene_go_webrtc/include",
        "-Ibazel-out/k8-fastbuild/bin/external/com_github_keroserene_go_webrtc/linux_amd64_stripped/go_default_library.cgo_codegen~",
    ],
    importpath = "github.com/keroserene/go-webrtc",
    visibility = ["//visibility:public"],
)

go_test(
    name = "go_default_test",
    srcs = [
        "configuration_test.go",
        "datachannel_test.go",
        "ice_test.go",
        "peerconnection_test.go",
        "sdp_test.go",
        "utils_test.go",
    ],
    embed = [":go_default_library"],
    deps = ["@com_github_smartystreets_goconvey//convey:go_default_library"],
)

@abergmeier
Copy link
Author

abergmeier commented May 17, 2018

The following does link:

load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

cc_import(
    name = "_precompiled",
    static_library = "lib/libwebrtc-linux-amd64-magic.a",
)

cc_library(
    name = "_lib",
    hdrs = glob(["include/**/*.h"]),
    defines = [
        "_GLIBCXX_USE_CXX11_ABI=0",
        "POSIX",
        "WEBRTC_LINUX",
    ],
    includes = [
        "include",
    ],
    deps = [
        ":_precompiled",
    ],
)

go_library(
    name = "go_default_library",
    srcs = [
        "configuration.go",
        "ctestenums.cc",
        "ctestenums.h",
        "datachannel.cc",
        "datachannel.go",
        "datachannel.h",
        "datachannel.hpp",
        "ice.go",
        "logging.go",
        "peerconnection.cc",
        "peerconnection.go",
        "peerconnection.h",
        "sdp.go",
        "utils.go",
    ],
    cgo = True,
    clinkopts = ["-ldl", "-lX11", "-lrt"],
    cxxopts = [
        "-std=c++0x",
        "-Ibazel-out/k8-fastbuild/bin/external/com_github_keroserene_go_webrtc/linux_amd64_stripped/go_default_library.cgo_codegen~",
    ],
    cdeps = [
        ":_lib",
    ],
    importpath = "github.com/keroserene/go-webrtc",
    visibility = ["//visibility:public"],
)

go_test(
    name = "go_default_test",
    srcs = [
        "configuration_test.go",
        "datachannel_test.go",
        "ice_test.go",
        "peerconnection_test.go",
        "sdp_test.go",
        "utils_test.go",
    ],
    embed = [":go_default_library"],
    deps = ["@com_github_smartystreets_goconvey//convey:go_default_library"],
)

@jayconrod
Copy link
Contributor

I'll look into this as soon as I can, but due to conference and travel schedules, it will be at least a few days before I can look into this.

A few things I notice from the build files:

  • glob(["include/**/*.h"]) - Gazelle will only ever consider files in the same directory, so it won't emit globs like this. Also note: glob only reaches down as far as the next build file.
  • Many compile and link flags can be pulled in through #cgo CFLAGS: ... directives. Gazelle will attempt to correct paths in these to be relative to the package, rather than the repository root (which is what Bazel expects when it invokes C/C++ compilers).
  • Take care when including or linking non-standard system libraries like -lX11. Probably unavoidable, since converting all these to Bazel would be time consuming, but you won't get a reproducible build.
  • You might be interested in the macro go_binary_c_archive_shared though this probably doesn't apply to your use case. It kicks in with c_archive or c_shared modes. We should expose _cgo_exports.h in a way where it can be more easily consumed by other cgo code in the normal link mode.

@abergmeier
Copy link
Author

  • glob(["include/**/*.h"]) - Gazelle will only ever consider files in the same directory, so it won't emit globs like this. Also note: glob only reaches down as far as the next build file.

Not just in this case I would consider this a bug. With the default go toolchain, there is no visibility and thus everything is "visible" no matter how deep the files are nested. As such, IMO gazelle should emulate that behavior as much as possible (and as a consequence collect these header files).
The note for package borders is an obvious limitation but no reason not to include deeper nested files.

  • Many compile and link flags can be pulled in through #cgo CFLAGS: ... directives. Gazelle will attempt to correct paths in these to be relative to the package, rather than the repository root (which is what Bazel expects when it invokes C/C++ compilers).

That is great. Sadly this project does not actually use this mechanism.
I was quite surprised to find that they use the pc mechanism and it seems like the go toolchain does something automagical here. And now also a bit shocked.

  • Take care when including or linking non-standard system libraries like -lX11. Probably unavoidable, since converting all these to Bazel would be time consuming, but you won't get a reproducible build.

Agreed.

  1. the problem might be less with time consumption but more with licensing. All things that this project links dynamically is actually *GPL.
  2. For this particular library I would even consider needing X11 a bug.
  • You might be interested in the macro go_binary_c_archive_shared though this probably doesn't apply to your use case. It kicks in with c_archive or c_shared modes. We should expose _cgo_exports.h in a way where it can be more easily consumed by other cgo code in the normal link mode.

Thanks, will give that a spin next week.

@jayconrod jayconrod changed the title cgo include paths not sufficient Read flags from pkg-config files May 23, 2018
@jayconrod
Copy link
Contributor

Ok, I had a chance to look at this. All the flags needed are in the .pc files, and Gazelle doesn't know about these. I was vaguely aware that cgo supported them. I've marked this issue as a feature request for Gazelle to support them as well.

In general, cgo flags almost always require some adjustment in Bazel because they frequently point to include directories and libraries outside of the workspace. Even link flags within the workspace like -lwebrtc-linux-amd64-magic in this repo) need some translation, and Gazelle won't be able to do that.

Unfortunately, those adjustments are hard to make with go_repository. At the moment, the best thing I can point you to is Gazelle's variant of git_repository and http_archive. These work like the native versions, but they let you copy in a pregenerated set of build files. There's not a convenient way to generate these (yet; I plan to automate this soon). But you can start with go_repository, then copy the build files out of $(bazel info output_base)/external/com_github_keroserene_go_webrtc, modify them as needed, and check them in somewhere. Gazelle and rules_go use these rules for their dependencies. See deps.bzl for an example.

@mariusgrigoriu
Copy link

Gazelle errors out when it encounters pkg-config today. Creating BUILD files by hand can be challenging when dealing with many external or vendored repositories that use pkg-config. Does it make sense to throw a warning instead so we can get a set of BUILD files that we can edit and add C flags to? (Encountering this when attempting to build CRI-O with rules_go.)

@jayconrod
Copy link
Contributor

Closing this issue: I don't think pkg-config can be reasonably supported. It fundamentally relies on system configuration outside the Bazel workspace. Flags appropriate for one system may not be appropriate for another. Different versions of a library may be installed (or not installed at all). Cross compilation and remote execution can't work.

A better way to depend on these libraries is to pull them in as git_repository or http_archive rules with build files that provide cc_library targets. Unfortunately, there's not an easy way to automate that.

@fengye87
Copy link

A better way to depend on these libraries is to pull them in as git_repository or http_archive rules with build files that provide cc_library targets. Unfortunately, there's not an easy way to automate that.

@jayconrod Could you give some more details here? I'm migrating a Go project to Bazel, which requires libvirt-go as a dependency. I used to yum install -y libvirt-devel and go build would just run as I'd expect. Now under Bazel, it keeps complaining about undefined reference to 'virXXXX'. So I understand that I must somehow introduce the libvirt lib to the Bazel workspace, but just couldn't make it work with cc_library. Some directions would really help me here!

BTW, libvirt-go has #cgo pkg-config: libvirt directives in its source code, and I can live with "Cross compilation and remote execution can't work" thing.

@jayconrod
Copy link
Contributor

You'll need to be able to incorporate libvirt into a Bazel build. There are a number of ways to do that. I have no familiarity with that library specifically, but in general for C/C++ projects:

  • To build from source, use an http_archive or git_repository and provide a build file or patch containing multiple build files that define cc_library targets.
  • To use binaries, you can extract the rpm somewhere, download with http_archive or git_repository, and provide a build file or patch with a cc_import target.
  • Instead of http_archive and git_repository, you could vendor files into your project directly.
  • If you want to use the package on your local system, you could define a repository rule that copies and configures it, or you could define a cc_toolchain that includes it. I really can't recommend this approach though: your builds won't be repeatable for anyone else.

@fengye87
Copy link

fengye87 commented Jul 1, 2020

@jayconrod Thank you for your quick response. So I finally got it work with your last option, posting some key points here in case someone may also need it.

First, introduce the libvirt library to the workspace:

# WORKSPACE
new_local_repository(
    name = "system_libs",
    build_file_content = """
load("@rules_cc//cc:defs.bzl", "cc_library")

cc_library(
    name = "libvirt",
    srcs = ["libvirt.so", "libvirt-admin.so", "libvirt-lxc.so", "libvirt-qemu.so"],
    visibility = ["//visibility:public"],
)
""",
    path = "/usr/lib64",
)

Patch the auto generated BUILD.bazel file of libvirt-go to add the lib with cdeps:

# WORKSPACE
go_repository(
    name = "com_github_libvirt_libvirt_go",
    importpath = "github.com/libvirt/libvirt-go",
    patch_args = ["-p1"],
    patches = ["//hack:com_github_libvirt_libvirt_go.patch"],
    sum = "h1:/Vmyq7XnPS2bfjJUkeJrkDueFDXNtbIkIE/E0tfRlY8=",
    version = "v6.4.0+incompatible",
)
# hack/com_github_libvirt_libvirt_go.patch
diff --git a/BUILD.bazel b/BUILD.bazel
index ce3e7e0..442f16b 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -92,6 +92,7 @@ go_library(
         "typedparams_wrapper.go",
         "typedparams_wrapper.h",
     ],
+    cdeps = ["@system_libs//:libvirt"],
     cgo = True,
     importpath = "github.com/libvirt/libvirt-go",
     visibility = ["//visibility:public"],

With these in place, I can now build and run my Go binary. This is not the ideal way to go since it would require people to install libvirt as system lib to compile or run the program, but I'm glad it's working. I'd probably vendor these .so files in the next step to make builds repeatable again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants