Migrate to cc_common for collecting C/C++ paths and options (#1768)

Previously, the internal go_context_data read C/C++ paths and options from the CcToolchainInfo provider. Most of the fields in this provider come from CROSSTOOL and will be removed in Bazel 0.20.0 (see https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#disable-legacy-c-toolchain-api). This change migrates us to use the new cc_common module. This change is intended to be a quick fix to be backported to old release branches, so no architectural changes are made. Fixes #1744 Related #1742
bazelbuild · Oct 16, 2018 · c414e7c · c414e7c
1 parent b6dd741
commit c414e7c
Show file tree

Hide file tree

Showing 9 changed files with 225 additions and 46 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -11,7 +11,7 @@ os:
   - osx
 
 env:
-  - V=0.16.0
+  - V=0.17.2
 
 before_install:
   - |

diff --git a/go/platform/apple.bzl b/go/platform/apple.bzl
@@ -29,7 +29,7 @@ PLATFORMS = {
 def _apple_version_min(platform, version):
     return "-m" + platform.name_in_plist.lower() + "-version-min=" + version
 
-def apple_ensure_options(ctx, env, tags, compiler_options, linker_options, target_gnu_system_name):
+def apple_ensure_options(ctx, env, tags, compiler_option_lists, linker_option_lists, target_gnu_system_name):
     """apple_ensure_options ensures that, when building an Apple target, the
     proper environment, compiler flags and Go tags are correctly set."""
     platform = PLATFORMS.get(target_gnu_system_name)
@@ -39,7 +39,9 @@ def apple_ensure_options(ctx, env, tags, compiler_options, linker_options, targe
         tags.append("ios")  # needed for stdlib building
     if platform in [apple_common.platform.ios_device, apple_common.platform.ios_simulator]:
         min_version = _apple_version_min(platform, "7.0")
-        compiler_options.append(min_version)
-        linker_options.append(min_version)
+        for compiler_options in compiler_option_lists:
+            compiler_options.append(min_version)
+        for linker_options in linker_option_lists:
+            linker_options.append(min_version)
     xcode_config = ctx.attr._xcode_config[apple_common.XcodeVersionConfig]
     env.update(apple_common.target_apple_env(xcode_config, platform))
diff --git a/go/private/actions/link.bzl b/go/private/actions/link.bzl
@@ -22,6 +22,8 @@ load(
     "@io_bazel_rules_go//go/private:mode.bzl",
     "LINKMODE_NORMAL",
     "LINKMODE_PLUGIN",
+    "extld_from_cc_toolchain",
+    "extldflags_from_cc_toolchain",
 )
 load(
     "@io_bazel_rules_go//go/private:skylib/lib/shell.bzl",
@@ -69,7 +71,7 @@ def emit_link(
     # Exclude -lstdc++ from link options. We don't want to link against it
     # unless we actually have some C++ code. _cgo_codegen will include it
     # in archives via CGO_LDFLAGS if it's needed.
-    extldflags = [f for f in go.cgo_tools.linker_options if f not in ("-lstdc++", "-lc++")]
+    extldflags = [f for f in extldflags_from_cc_toolchain(go) if f not in ("-lstdc++", "-lc++")]
 
     if go.coverage_enabled:
         extldflags.append("--coverage")
@@ -78,8 +80,7 @@ def emit_link(
     tool_args = go.tool_args(go)
 
     # Add in any mode specific behaviours
-    extld = go.cgo_tools.compiler_executable
-    tool_args.add_all(["-extld", extld])
+    tool_args.add_all(extld_from_cc_toolchain(go))
     if go.mode.race:
         tool_args.add("-race")
     if go.mode.msan:
@@ -142,6 +143,7 @@ def emit_link(
     builder_args.add_all(["-main", archive.data.file])
     tool_args.add_all(gc_linkopts)
     tool_args.add_all(go.toolchain.flags.link)
+
     # Do not remove, somehow this is needed when building for darwin/arm only.
     tool_args.add("-buildid=redacted")
     if go.mode.strip:
@@ -179,12 +181,7 @@ def _bootstrap_link(go, archive, executable, gc_linkopts):
         arguments = [args],
         mnemonic = "GoLink",
         command = "export GOROOT=\"$(pwd)\"/{} && {} \"$@\"".format(shell.quote(go.root), shell.quote(go.go.path)),
-        env = {
-            # workaround: go link tool needs some features of gcc to complete the job on Arm platform.
-            # So, PATH for 'gcc' is required here on Arm platform.
-            "PATH": go.cgo_tools.compiler_path,
-            "GOROOT_FINAL": "GOROOT",
-        },
+        env = {"GOROOT_FINAL": "GOROOT"},
     )
 
 def _extract_extldflags(gc_linkopts, extldflags):

diff --git a/go/private/context.bzl b/go/private/context.bzl
@@ -16,6 +16,14 @@ load(
     "@bazel_tools//tools/cpp:toolchain_utils.bzl",
     "find_cpp_toolchain",
 )
+load(
+    "@bazel_tools//tools/build_defs/cc:action_names.bzl",
+    "CPP_COMPILE_ACTION_NAME",
+    "CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME",
+    "CPP_LINK_EXECUTABLE_ACTION_NAME",
+    "CPP_LINK_STATIC_LIBRARY_ACTION_NAME",
+    "C_COMPILE_ACTION_NAME",
+)
 load(
     "@io_bazel_rules_go//go/private:providers.bzl",
     "EXPLICIT_PATH",
@@ -53,6 +61,7 @@ load(
 )
 
 GoContext = provider()
+_GoContextData = provider()
 
 _COMPILER_OPTIONS_BLACKLIST = {
     "-fcolor-diagnostics": None,
@@ -225,7 +234,7 @@ def go_context(ctx, attr = None):
 
     host_only = getattr(attr, "_hostonly", False)
 
-    context_data = attr._go_context_data
+    context_data = attr._go_context_data[_GoContextData]
     mode = get_mode(ctx, host_only, toolchain, context_data)
     tags = list(context_data.tags)
     if mode.race:
@@ -248,7 +257,6 @@ def go_context(ctx, attr = None):
         "GOROOT": goroot,
         "GOROOT_FINAL": "GOROOT",
         "CGO_ENABLED": "0" if mode.pure else "1",
-        "PATH": context_data.cgo_tools.compiler_path,
     })
 
     # TODO(jayconrod): remove this. It's way too broad. Everything should
@@ -308,42 +316,178 @@ def go_context(ctx, attr = None):
         _ctx = ctx,  # TODO: All uses of this should be removed
     )
 
-def _go_context_data(ctx):
-    cpp = find_cpp_toolchain(ctx)
-    features = ctx.features
-    compiler_options = _filter_options(
-        cpp.compiler_options() + cpp.unfiltered_compiler_options(features),
+def _go_context_data_impl(ctx):
+    # TODO(jayconrod): find a way to get a list of files that comprise the
+    # toolchain (to be inputs into actions that need it).
+    # ctx.files._cc_toolchain won't work when cc toolchain resolution
+    # is switched on.
+    cc_toolchain = find_cpp_toolchain(ctx)
+    feature_configuration = cc_common.configure_features(
+        cc_toolchain = cc_toolchain,
+        requested_features = ctx.features,
+        unsupported_features = ctx.disabled_features,
+    )
+
+    # TODO(jayconrod): keep the environment separate for different actions.
+    env = {}
+
+    c_compile_variables = cc_common.create_compile_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+    )
+    c_compiler_path = cc_common.get_tool_for_action(
+        feature_configuration = feature_configuration,
+        action_name = C_COMPILE_ACTION_NAME,
+    )
+    c_compile_options = _filter_options(
+        cc_common.get_memory_inefficient_command_line(
+            feature_configuration = feature_configuration,
+            action_name = C_COMPILE_ACTION_NAME,
+            variables = c_compile_variables,
+        ),
         _COMPILER_OPTIONS_BLACKLIST,
     )
-    linker_options = _filter_options(
-        cpp.mostly_static_link_options(False),
+    env.update(cc_common.get_environment_variables(
+        feature_configuration = feature_configuration,
+        action_name = C_COMPILE_ACTION_NAME,
+        variables = c_compile_variables,
+    ))
+
+    cxx_compile_variables = cc_common.create_compile_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+    )
+    cxx_compile_options = _filter_options(
+        cc_common.get_memory_inefficient_command_line(
+            feature_configuration = feature_configuration,
+            action_name = CPP_COMPILE_ACTION_NAME,
+            variables = cxx_compile_variables,
+        ),
+        _COMPILER_OPTIONS_BLACKLIST,
+    )
+    env.update(cc_common.get_environment_variables(
+        feature_configuration = feature_configuration,
+        action_name = CPP_COMPILE_ACTION_NAME,
+        variables = cxx_compile_variables,
+    ))
+
+    ld_executable_variables = cc_common.create_link_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+        is_linking_dynamic_library = False,
+    )
+    ld_executable_path = cc_common.get_tool_for_action(
+        feature_configuration = feature_configuration,
+        action_name = CPP_LINK_EXECUTABLE_ACTION_NAME,
+    )
+    ld_executable_options = _filter_options(
+        cc_common.get_memory_inefficient_command_line(
+            feature_configuration = feature_configuration,
+            action_name = CPP_LINK_EXECUTABLE_ACTION_NAME,
+            variables = ld_executable_variables,
+        ),
         _LINKER_OPTIONS_BLACKLIST,
     )
+    env.update(cc_common.get_environment_variables(
+        feature_configuration = feature_configuration,
+        action_name = CPP_LINK_EXECUTABLE_ACTION_NAME,
+        variables = ld_executable_variables,
+    ))
+
+    # We don't collect options for static libraries. Go always links with
+    # "ar" in "c-archive" mode. We can set the ar executable path with
+    # -extar, but the options are hard-coded to something like -q -c -s.
+    ld_static_lib_variables = cc_common.create_link_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+        is_linking_dynamic_library = False,
+    )
+    ld_static_lib_path = cc_common.get_tool_for_action(
+        feature_configuration = feature_configuration,
+        action_name = CPP_LINK_STATIC_LIBRARY_ACTION_NAME,
+    )
+    env.update(cc_common.get_environment_variables(
+        feature_configuration = feature_configuration,
+        action_name = CPP_LINK_STATIC_LIBRARY_ACTION_NAME,
+        variables = ld_static_lib_variables,
+    ))
+
+    ld_dynamic_lib_variables = cc_common.create_link_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+        is_linking_dynamic_library = True,
+    )
+    ld_dynamic_lib_path = cc_common.get_tool_for_action(
+        feature_configuration = feature_configuration,
+        action_name = CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME,
+    )
+    ld_dynamic_lib_options = _filter_options(
+        cc_common.get_memory_inefficient_command_line(
+            feature_configuration = feature_configuration,
+            action_name = CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME,
+            variables = ld_dynamic_lib_variables,
+        ),
+        _LINKER_OPTIONS_BLACKLIST,
+    )
+    env.update(cc_common.get_environment_variables(
+        feature_configuration = feature_configuration,
+        action_name = CPP_LINK_DYNAMIC_LIBRARY_ACTION_NAME,
+        variables = ld_dynamic_lib_variables,
+    ))
 
-    env = {}
     tags = []
     if "gotags" in ctx.var:
         tags = ctx.var["gotags"].split(",")
-    apple_ensure_options(ctx, env, tags, compiler_options, linker_options, cpp.target_gnu_system_name)
-    compiler_path, _ = cpp.ld_executable.rsplit("/", 1)
-    return struct(
+    apple_ensure_options(
+        ctx,
+        env,
+        tags,
+        (c_compile_options, cxx_compile_options),
+        (ld_executable_options, ld_dynamic_lib_options),
+        cc_toolchain.target_gnu_system_name,
+    )
+
+    # Add C toolchain directories to PATH.
+    # On ARM, go tool link uses some features of gcc to complete its work,
+    # so PATH is needed on ARM.
+    path_set = {}
+    if "PATH" in env:
+        for p in env["PATH"].split(ctx.configuration.host_path_separator):
+            path_set[p] = None
+    for tool_path in [c_compiler_path, ld_executable_path, ld_static_lib_path, ld_dynamic_lib_path]:
+        tool_dir, _, _ = tool_path.rpartition("/")
+        path_set[tool_dir] = None
+    paths = sorted(path_set.keys())
+    if ctx.configuration.host_path_separator == ":":
+        # HACK: ":" is a proxy for a UNIX-like host.
+        # The tools returned above may be bash scripts that reference commands
+        # in directories we might not otherwise include. For example,
+        # on macOS, wrapped_ar calls dirname.
+        if "/bin" not in path_set:
+            paths.append("/bin")
+        if "/usr/bin" not in path_set:
+            paths.append("/usr/bin")
+    env["PATH"] = ctx.configuration.host_path_separator.join(paths)
+
+    return [_GoContextData(
         strip = ctx.attr.strip,
         crosstool = ctx.files._cc_toolchain,
         tags = tags,
         env = env,
         cgo_tools = struct(
-            compiler_path = compiler_path,
-            compiler_executable = cpp.compiler_executable,
-            ld_executable = cpp.ld_executable,
-            compiler_options = compiler_options,
-            linker_options = linker_options,
-            options = compiler_options + linker_options,
-            c_options = cpp.c_options(),
+            c_compiler_path = c_compiler_path,
+            c_compile_options = c_compile_options,
+            cxx_compile_options = cxx_compile_options,
+            ld_executable_path = ld_executable_path,
+            ld_executable_options = ld_executable_options,
+            ld_static_lib_path = ld_static_lib_path,
+            ld_dynamic_lib_path = ld_dynamic_lib_path,
+            ld_dynamic_lib_options = ld_dynamic_lib_options,
         ),
-    )
+    )]
 
 go_context_data = rule(
-    _go_context_data,
+    _go_context_data_impl,
     attrs = {
         "strip": attr.string(mandatory = True),
         "_cc_toolchain": attr.label(default = "@bazel_tools//tools/cpp:current_cc_toolchain"),

diff --git a/go/private/mode.bzl b/go/private/mode.bzl
@@ -208,3 +208,30 @@ def link_mode_args(mode):
         if platform in _LINK_PLUGIN_PLATFORMS:
             args.append("-dynlink")
     return args
+
+def extldflags_from_cc_toolchain(go):
+    if go.mode.link in (LINKMODE_SHARED, LINKMODE_PLUGIN, LINKMODE_C_SHARED):
+        return go.cgo_tools.ld_dynamic_lib_options
+    else:
+        # NOTE: in c-archive mode, -extldflags are ignored by the linker.
+        # However, we still need to set them for cgo, which links a binary
+        # in each package. We use the executable options for this.
+        return go.cgo_tools.ld_executable_options
+
+def extld_from_cc_toolchain(go):
+    if go.mode.link in (LINKMODE_SHARED, LINKMODE_PLUGIN, LINKMODE_C_SHARED):
+        return ["-extld", go.cgo_tools.ld_dynamic_lib_path]
+    elif go.mode.link == LINKMODE_C_ARCHIVE:
+        if go.mode.goos == "darwin":
+            # TODO(jayconrod): on macOS, set -extar. At this time, wrapped_ar is
+            # a bash script without a shebang line, so we can't execute it. We
+            # use /usr/bin/ar (the default) instead.
+            return []
+        else:
+            return ["-extar", go.cgo_tools.ld_static_lib_path]
+    else:
+        # NOTE: In c-archive mode, we should probably set -extar. However,
+        # on macOS, Bazel returns wrapped_ar, which is not executable.
+        # /usr/bin/ar (the default) should be visible though, and we have a
+        # hack in link.go to strip out non-reproducible stuff.
+        return ["-extld", go.cgo_tools.ld_executable_path]
diff --git a/go/private/rules/cgo.bzl b/go/private/rules/cgo.bzl
@@ -39,13 +39,14 @@ load(
     "@io_bazel_rules_go//go/private:mode.bzl",
     "LINKMODE_C_ARCHIVE",
     "LINKMODE_C_SHARED",
+    "extldflags_from_cc_toolchain",
     "mode_string",
     "new_mode",
 )
 load(
     "@io_bazel_rules_go//go/platform:list.bzl",
-    "GOOS",
     "GOARCH",
+    "GOOS",
     "GOOS_GOARCH",
     "MSAN_GOOS_GOARCH",
     "RACE_GOOS_GOARCH",
@@ -143,9 +144,9 @@ def _cgo_codegen_impl(ctx):
     go = go_context(ctx)
     if not go.cgo_tools:
         fail("Go toolchain does not support cgo")
-    linkopts = go.cgo_tools.linker_options + ctx.attr.linkopts
-    cppopts = go.cgo_tools.compiler_options + ctx.attr.cppopts
-    copts = go.cgo_tools.c_options + ctx.attr.copts
+    linkopts = extldflags_from_cc_toolchain(go) + ctx.attr.linkopts
+    cppopts = list(ctx.attr.cppopts)
+    copts = go.cgo_tools.c_compile_options + ctx.attr.copts
     deps = depset([], order = "topological")
     cgo_export_h = go.declare_file(go, path = "_cgo_export.h")
     cgo_export_c = go.declare_file(go, path = "_cgo_export.c")
@@ -262,7 +263,7 @@ def _cgo_codegen_impl(ctx):
     # TODO(jayconrod): do we need to set this here, or only in _cgo_import?
     # go build does it here.
     env = go.env
-    env["CC"] = go.cgo_tools.compiler_executable
+    env["CC"] = go.cgo_tools.c_compiler_path
     env["CGO_LDFLAGS"] = " ".join(linkopts)
 
     cc_args.add_all(cppopts)