Split C and C++ compilation #1366

steeve · 2018-03-05T16:36:58Z

What it does

This PR splits C and C++ CGo compilation into separate steps. The main idea behind this is for specifying per language options (such as std=c++14 for C++).

For that, the pack builder has been modified to take multiple archives (C and C++) and fuse them.

This is also a required step for adding ObjC support.

I have also added a commit which tries to make relative CC options absolute (via whitelisting)

jayconrod · 2018-03-05T19:34:43Z

I took a quick look, but I won't be able to do a detailed review until tomorrow. I have a couple concerns before then though:

Could you explain a bit more about what the ObjC support will look like? I guess some sources would need to be compiled by objc_library or something like that.
Bazel treats the C / C++ toolchain as mostly one thing, configured with CROSSTOOL. I think Objective C / C++ is also configured with CROSSTOOL, but I don't know how the flags are separated. In general, I think CROSSTOOL is the right place for language-specific flags. go_library copts should be reserved for library-specific flags like -D, -I, -framework.
If C / C++ / ObjC sources and headers are separated into different cc_library / objc_library rules, how will they compile if there are dependencies between sources in different languages?
I mentioned this in the other PR, but there is ongoing work to expose C++ toolchains to Skylark. I'm not sure whether ObjC is part of that effort, but that would let us avoid cc_library and objc_library rules altogether. This is a big enough change that it might be better to wait until that work is complete.

steeve · 2018-03-05T21:48:55Z

Thanks for being so fast.

Here are some answers:

For ObjC support, you can check out the early work on this at https://github.com/znly/rules_go/commit/30ee77332fe195d6819c3b8ff6b9233b20ae985a, but meat is is at https://github.com/znly/rules_go/commit/30ee77332fe195d6819c3b8ff6b9233b20ae985a#diff-1d1e845efd5e9ec6b7a69dcecb01ed38R446
You're right, but if you're building something else than C, setting some flags will contaminate the CGoCodeGen pure C compilation flags and fail the whole thing. In our project, for instance, we're using C++14 in cpp files in Go packages, and setting CXXFLAGS is the only way to make it work. However, if we add -std=c++14 to a pure C compilation, clang will fail.
For now I'm propagating the deps to all the languages. I don't think it's that bad really? Wether or not one uses the deps is up to them but at least Bazel makes them available to the library.
I'd love for this to happen! It would be the cleanest solution for sure. Last I checked though the spec is expected to land for Q1, then there is the implementation. Wet finger estimate point to at least may-june before it's in master. Or maybe you have some insider information ? 😄

jayconrod · 2018-03-05T23:17:51Z

For c++14, it looks like the recommended way is to pass --cxxopt='-std=c++14' on the command line. You can put this in your tools/bazel.rc file so Bazel picks it up automatically. That will also apply to other cc_library rules.

About C/C++ sandwich: @mhlopko, how's it going? Will ObjC be supported, too? For context, it would be useful to build mixed C/C++/ObjC sources that appear alongside cgo code. Right now, we build C/C++ as a cc_library.

steeve · 2018-03-05T23:20:10Z

For c++14, it looks like the recommended way is to pass --cxxopt='-std=c++14' on the command line. You can put this in your tools/bazel.rc file so Bazel picks it up automatically. That will also apply to other cc_library rules.

Indeed, I saw that flag but I figure it was easier just splitting and doing it package by package and also because Go supports setting CFLAGS and CXXFLAGS separately, too.

jayconrod · 2018-03-06T20:36:59Z

I'll wait until #1365 is merged before diving into this.

steeve · 2018-03-06T21:50:54Z

Sure thing!

hlopko · 2018-03-07T10:07:55Z

Sorry for the slow reply. We are working on 2 things in C++ rules:

Skylark API to C++ toolchain (tracking issue) - so you can ask cc_toolchain to give you the same command line that cc_library/binary would use
Skylark API to C++ rules (C++ sandwich, tracking issue) - so you can access C++ providers and interoperate with c++ rules from other Skylark rules.

I don't know much about open source go rules, but I assume you'll want to use the sandwich. That will allow you to consume C++ providers, dig objects out, and use them with cgo. @oquenchil can comment on timeframes.

Re ObjC, they will use their own providers, but tbh I don't know much about that. Maybe @c-parsons has more information.

steeve · 2018-03-07T11:08:59Z

@mhlopko thanks for the reply

Indeed we'd like to be able to return cc providers to be consumed by cc_ rules. Right now we rely on macros and filegroups to do so, and in the end are forced to name them differently.

See for instance at https://github.com/bazelbuild/rules_go/pull/1365/files#diff-dfaef6484291891479c192a6169bc232R90

steeve · 2018-03-07T13:18:09Z

Changed pack to ensure object names are unique among the multiple archives in dd11da3

jayconrod · 2018-03-08T19:57:02Z

@mhlopko I'm actually most interested in the toolchain being exposed in Skylark. How far along is that? #4571 doesn't have much information. We'd like to be able to invoke a C / C++ / ObjC compiler and linker on some generated code during execution without creating cc_library / objc_library rules. I think we can get the compiler command and flags right now, but I'm guessing the includes and libraries and other toolchain files won't be visible in the sandbox.

The sandwich is also useful for other things, but I'm mainly interested in the toolchain right now.

steeve · 2018-03-08T21:56:36Z

Correct me if I'm wrong, but why not do a pure skylark cc_ rules when #4571 lands ?

jayconrod · 2018-03-08T22:00:13Z

I think that's the long-term plan, but the C/C++ rules are really complicated, having accumulated a decade's worth of features and idiosyncrasies within Google. A full migration will take years. I hope a simplified version that serves most common cases will be available sooner than that though.

hlopko · 2018-03-09T07:30:16Z

Steeve: that's the end goal, to migrate existing rules into Skylark. But there are things that C++ rules do that are not supported in Skylark yet, plus we want to use the same rules internally and externally, and for that we need to do small incremental steps, making sure google doesn't break.

Jay: Since I still know very little about external go rules let me make sure at least you have enough information to decide what's best :) Part of the sandwich (spoiler alert :) are functions cc_common.compile(...) and cc_common.link(...). AFAIK this is what you need, give C++ rules sources, compile them, make a .a of them, and propagate providers. Toolchain api will only enable you to get the command line flags that C++ rules would use. That's definitely something you need for internal go rules though.

The work is ongoing, optimistically EOQ1 for test runs behind a whitelist. I'll keep you updated.

oquenchil · 2018-03-14T12:38:36Z

The work is ongoing, optimistically EOQ1 for test runs behind a whitelist. I'll keep you updated.

This timeframe sounds correct. This should be tested internally first where we have more control on who's using the API and we can change it more easily if necessary.

steeve · 2018-03-23T13:56:46Z

Please hold on this one, I'm going to properly rebase it on top of master.

Signed-off-by: Steeve Morin <[email protected]>

steeve · 2018-04-09T22:02:25Z

I've properly rebased it and added tests.
@jayconrod your move

steeve · 2018-04-09T22:20:09Z

The test failure is due to the Travis machine apparently not having a C++14 compiler installed.

https://travis-ci.org/bazelbuild/rules_go/jobs/363343765#L2495

ERROR: /home/travis/build/bazelbuild/rules_go/tests/core/cxx/BUILD.bazel:3:1: C++ compilation of rule '//tests/core/cxx:go_default_library.cgo_cxx_lib' failed (Exit 1): gcc failed: error executing command

https://travis-ci.org/bazelbuild/rules_go/jobs/363343765#L2504

gcc: error: unrecognized command line option '-std=c++14'

I can change the test to use C++11. But I was using C++14 specific feaures to highlight it, but oh well.

jayconrod

Thanks for rebasing and sorry for the slow response; I just got back from vacation.

I have a few comments on specifics, but this seems like the right approach given our current constraints. I hope that when Bazel lets us build C/C++/ObjC command lines, we can simplify this and stop using separate cc_library rules.

jayconrod · 2018-04-12T17:37:04Z

go/private/rules/cgo.bzl

@@ -283,7 +295,7 @@ def _cgo_collect_info_impl(ctx):
          gen_go_srcs = codegen.gen_go + import_files,
          cgo_deps = codegen.deps,
          cgo_exports = codegen.exports,
-          cgo_archive = _select_archive(ctx.files.lib),
+          cgo_archives = _select_archives(ctx.files.libs),


Please update documentation for GoSource in go/providers.rst to reflect this change.

FYI, I'm okay with small API breaking changes like this in providers between major versions. I doubt many people are using this yet. In the future, we may need to be more strict about this though.

jayconrod · 2018-04-12T17:38:21Z

go/private/actions/pack.bzl

@@ -16,7 +16,7 @@ def emit_pack(go,
    in_lib = None,
    out_lib = None,
    objects = [],
-    archive = None):
+    archives = []):


Please update documentation in go/toolchains.rst#pack to reflect this change.

jayconrod · 2018-04-12T17:45:25Z

go/private/rules/cgo.bzl

@@ -78,20 +78,23 @@ def _c_filter_options(options, blacklist):
  return [opt for opt in options
        if not any([opt.startswith(prefix) for prefix in blacklist])]

-def _select_archive(files):
+def _select_archives(files):


I don't think this is safe. See the documentation below. cc_library may produce multiple output files, and we want exactly one of them.

Instead of calling this once on ctx.files.libs, call for each target, i.e., [_select_archive(lib.files) for lib in ctx.attr.libs]

i still need to check that one out

ok got it, i've pushed a fix

jayconrod · 2018-04-12T17:50:41Z

go/private/rules/cgo.bzl

@@ -325,13 +337,17 @@ def setup_cgo_library(name, srcs, cdeps, copts, clinkopts):
      "external/" + REPOSITORY_NAME[1:] if len(REPOSITORY_NAME) > 1 else "",
      PACKAGE_NAME)
  copts = copts + ["-I", base_dir]
+  cxxopts = cxxopts + ["-I", base_dir]
+  cppopts = cppopts + ["-I", base_dir]


Since we're separating copts and cppopts, this flag should only be added to cppopts.

Please also double-check all mentions of copts in this file. There are several places in _cgo_codegen_impl where it should be changed to cppopts. Basically anything that isn't specific to C or C++ should be changed.

jayconrod · 2018-04-12T18:01:21Z

go/private/rules/wrappers.bzl

@@ -25,6 +25,8 @@ _CGO_ATTRS = {
    "srcs": None,
    "cdeps": [],
    "copts": [],
+    "cxxopts": [],
+    "cppopts": [],


Please update attribute tables in go/core.rst to include the new attributes.

This may break people that have C++ code that currently depends on options in copts. Hopefully this won't affect many people. I'll file an issue on Gazelle to separate these.

jayconrod · 2018-04-12T18:13:56Z

tests/core/cxx/BUILD.bazel

@@ -0,0 +1,19 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")


Several test comments:

In terms of test organization, I'd prefer this to be tests/core/cgo, since this is really testing cgo functionality. Could be divided further into language subdirectories (tests/core/cgo/cxx).

We really want to verify that the right set of options get passed to each language. copts should be C-only, cxxopts should be C++-only, cppopts should be both. Easiest way to do that is probably with -D flags, then checking at build time with #if and #error. This would also fix the error on Travis CI due not having a C++14 compiler available.

Please create a README.rst describing the tests, and run tests/update.py.

steeve · 2018-04-15T16:15:30Z

I'm in a bit of a dilemma over copts vs cppopts. When one wants to use -D they now need to put it into cppopts or else it won't work. While is this correct, this is rather not friendly imho...

steeve · 2018-04-15T16:17:05Z

PR updated with most of the advices.
See 8ee185c for what I'm describing

steeve · 2018-04-16T11:37:10Z

Never mind I think I solved it by a41c767

jayconrod

Getting close. A few more minor comments on changes.

jayconrod · 2018-04-17T17:02:56Z

go/private/rules/cgo.bzl

+  outs = []
+  for lib in libs:
+    archive = _select_archive(lib.files)
+    if archive:


When would a cc_library not produce any usable archives? Does that happen if there are no source files?

This can happen if there are only header files (which happens if C++ files are missing, for instance).

I realize I could have been clearer. In the case of a regular C compilation, the cc_library responsible of building the C++ files will not produce any archive.

jayconrod · 2018-04-17T17:10:02Z

go/private/rules/cgo.bzl


 def _cgo_codegen_impl(ctx):
  go = go_context(ctx)
  if not go.cgo_tools:
    fail("Go toolchain does not support cgo")
  linkopts = ctx.attr.linkopts[:]
-  copts = go.cgo_tools.c_options + go.cgo_tools.compiler_options + ctx.attr.copts
+  cppopts = go.cgo_tools.c_options + go.cgo_tools.compiler_options + ctx.attr.copts + ctx.attr.cppopts


Let's keep cppopts and copts separate here for clarity.

cppopts = go.cgo_tools.compiler_options +ctx.attr.cppopts copts = go.cgo_tools.c_options + ctx.attr.copts

jayconrod · 2018-04-17T17:10:27Z

go/private/rules/cgo.bzl

@@ -161,10 +181,10 @@ def _cgo_codegen_impl(ctx):
  # The first -- below is to stop the cgo from processing args, the
  # second is an actual arg to forward to the underlying go tool
  args.add(["--", "--"])
-  args.add(copts)
+  args.add(cppopts)


Both cppopts and copts can be added here.

jayconrod · 2018-04-17T17:12:33Z

tests/core/cgo/BUILD.bazel

+    cgo = True,
+    copts = ["-DRULES_GO_C"],
+    cxxopts = ["-DRULES_GO_CPP"],
+    srcs = [


Define something in cppopts and check that define is visible in both add.c and add.cpp.

jayconrod · 2018-04-17T17:14:19Z

go/core.rst

@@ -392,6 +392,18 @@ Attributes
 | Subject to `"Make variable"`_ substitution and `Bourne shell tokenization`_.                     |
 | Only valid if :param:`cgo` = :value:`True`.                                                      |
 +----------------------------+-----------------------------+---------------------------------------+
+| :param:`cxxopts`           | :type:`string_list`         | :value:`[]`                           |


Looks good, but copy to go_library and go_test.

Signed-off-by: Steeve Morin <[email protected]>

…one) Signed-off-by: Steeve Morin <[email protected]>

Signed-off-by: Steeve Morin <[email protected]>

steeve · 2018-04-17T21:49:40Z

I've updated the PR, thank you for the feedback!

steeve · 2018-04-18T17:18:19Z

@jayconrod do you want me to rebase instead ?

jayconrod · 2018-04-18T17:33:10Z

@steeve No need, I was just waiting for CI to run again after resolving the merge conflict.

Thanks again for working on this.

steeve requested review from ianthehat and jayconrod as code owners March 5, 2018 16:36

googlebot added the cla: yes label Mar 5, 2018

steeve added 3 commits April 6, 2018 23:31

Allow multiple archives when packing

253b2d6

Signed-off-by: Steeve Morin <[email protected]>

Split CGo C and C++ options and flags

b7d8a8b

Signed-off-by: Steeve Morin <[email protected]>

Add C++ tests

eda7f26

Signed-off-by: Steeve Morin <[email protected]>

steeve mentioned this pull request Apr 9, 2018

Add ObjC support #1437

Merged

jayconrod mentioned this pull request Apr 12, 2018

Split cgo options by language bazelbuild/bazel-gazelle#180

Open

jayconrod reviewed Apr 12, 2018

View reviewed changes

jayconrod reviewed Apr 17, 2018

View reviewed changes

steeve added 5 commits April 17, 2018 23:44

Update documentation

9e92da8

Signed-off-by: Steeve Morin <[email protected]>

Set preprocessor related opts to cppopts and split them from copts

76e73a1

Signed-off-by: Steeve Morin <[email protected]>

Check for C/C++ opts split via defines.

5614b4d

Signed-off-by: Steeve Morin <[email protected]>

Secure _select_archives if cc_library outputs mutliple archives (or n…

53ab130

…one) Signed-off-by: Steeve Morin <[email protected]>

Add cppopts to the test

d7b39f9

Signed-off-by: Steeve Morin <[email protected]>

Merge branch 'master' into feature/cxx

137685e

jayconrod merged commit dcaae02 into bazelbuild:master Apr 18, 2018

		@@ -0,0 +1,19 @@
		load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

Split C and C++ compilation #1366

Split C and C++ compilation #1366

Conversation

steeve commented Mar 5, 2018 • edited Loading

What it does

jayconrod commented Mar 5, 2018

steeve commented Mar 5, 2018

jayconrod commented Mar 5, 2018

steeve commented Mar 5, 2018 • edited Loading

jayconrod commented Mar 6, 2018

steeve commented Mar 6, 2018

hlopko commented Mar 7, 2018

steeve commented Mar 7, 2018

steeve commented Mar 7, 2018

jayconrod commented Mar 8, 2018

steeve commented Mar 8, 2018

jayconrod commented Mar 8, 2018

hlopko commented Mar 9, 2018

oquenchil commented Mar 14, 2018

steeve commented Mar 23, 2018

steeve commented Apr 9, 2018

steeve commented Apr 9, 2018

jayconrod 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

steeve commented Apr 15, 2018

steeve commented Apr 15, 2018

steeve commented Apr 16, 2018

jayconrod 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

steeve commented Apr 17, 2018

steeve commented Apr 18, 2018

jayconrod commented Apr 18, 2018

steeve commented Mar 5, 2018 •

edited

Loading

steeve commented Mar 5, 2018 •

edited

Loading