Migrate rules_rust to the new Starlark C++ toolchain API #133

hlopko · 2018-10-19T12:12:17Z

This is needed for rules_rust to be forward compatible with Bazel 0.20.
Tracking issues for migration:

Fixes #131.

This is needed for rules_rust to be forward compatible with Bazel 0.20. Tracking issues for migration: * bazelbuild/bazel#6380 * bazelbuild/bazel#6434 Fixes #131.

hlopko · 2018-10-19T12:36:42Z

Tests pass with Bazel 0.18, for some reason Bazel ci still tests with bazel 0.17 on windows and ubuntu.

hlopko · 2018-10-19T13:32:21Z

Ah, I just discovered https://github.com/bazelbuild/bazel-skylib/blob/master/lib/versions.bzl. Let me rewrite the version checking logic with that

hlopko · 2018-10-19T16:17:31Z

And 10 commits later, we're green. Please take a look now. Thank you!

acmcarther

Thanks for implementing this!

I've got a couple of broad questions primarily around how the cpp toolchain is located since this may need to be tweaked to support cross compilation in the future.

acmcarther · 2018-10-19T16:42:38Z

rust/private/rustc.bzl

-    cc = cpp_fragment.compiler_executable
-    ar = cpp_fragment.ar_executable
+    cc_toolchain = find_cpp_toolchain(ctx)
+    feature_configuration = cc_common.configure_features(


Where is cc_common defined and how does it tie into cc_toolchain?

cc_common is currently builtin, the list of all of the builtins is here: https://docs.bazel.build/versions/master/skylark/lib/skylark-overview.html.

Hmm, I'm confused by github review, didn't I answer this already? :) cc_common is a builtin Starlark module, the list of all is here: https://docs.bazel.build/versions/master/skylark/lib/skylark-overview.html

acmcarther · 2018-10-19T16:43:41Z

rust/private/rustc.bzl

+        requested_features = ctx.features,
+        unsupported_features = ctx.disabled_features,
+    )
+    ld = cc_common.get_tool_for_action(


Tangentially related to this PR:

Its not clear to me how the linker gets selected. Is the target platform the same as the one specified (indirectly) in find_cpp_toolchain?

C++ rules don't currently use platforms, they have their own thing controlled by --crosstool_top, --cpu, and --compiler (and their corresponding host variants, such as --host_crosstool_top. Here in the rule we are getting target C++ toolchain, when we're in the target configuration, and host toolchain when in host configuration (so if somebody just tells Bazel to build //foo, we build foo in target configuration. If somebody writes a genrule that has foo as a tool, Bazel will build it in host configuration). Did I answer your question?

C++ rules don't currently use platforms, they have their own thing controlled by --crosstool_top, --cpu, and --compiler (and their corresponding host variants, such as --host_crosstool_top. Here in the rule we are getting target C++ toolchain, when we're in the target configuration, and host toolchain when in host configuration (so if somebody just tells Bazel to build //foo, we build foo in target configuration. If somebody writes a genrule that has foo as a tool, Bazel will build it in host configuration). Did I answer your question?

acmcarther · 2018-10-19T16:44:35Z

rust/private/rustc.bzl

@@ -191,8 +235,6 @@ def rustc_compile_action(
    if ar_str.find("libtool", 0) != -1:
        ar = "/usr/bin/ar"


Is this old ar finding logic obsolete with your changes?

Looks like this can be removed

>> rustc --version rustc 1.26.1 (827013a31 2018-05-25) >> rustc -C help ... -C ar=val -- this option is deprecated and does nothing

Removed. We will have to force ar and other tools to rustc when we want to be fully host-independent. Which we will have to be to support remote execution.

To be clear, it can (and will) happen that C++ toolchain will suggest to use libtool on darwin.

mfarrugi

LGTM, some questions on the linker toolchain api.

Are "fragments" going to be replaced as well, or is that a separate concept to toolchains?

mfarrugi · 2018-10-19T19:39:26Z

rust/private/rustc.bzl

+    link_variables = cc_common.create_link_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+        is_linking_dynamic_library = False,


A couple of crate types will produce dynamic libraries, should this account for that?

rules_rust/rust/private/rust.bzl

Lines 32 to 33 in d33bf1d

if crate_type in ("dylib", "cdylib", "proc-macro"):

extension = toolchain.dylib_ext

Ideally we would ask C++ toolchain for options twice, once for executable, once for dynamic library. In practice, they usually only differ in dynamic library having --shared, which rustc might be initiative enough to add on its own. But I don't know tbh.

If we have rust_library(..., crate_type = "cdylib") should this be set to True?
When would we be asking the toolchain for options twice?

mfarrugi · 2018-10-19T19:40:41Z

rust/private/rustc.bzl

-    return [
-        "$ORIGIN/" + relative_path(output_dir, lib_dir)
+    return depset([
+        relative_path(output_dir, lib_dir)
        for lib_dir in _get_dir_names(depinfo.transitive_dylibs)


If the toolchain handles $ORIGIN prefixes, might it handle relativizing the paths? I feel like it must

Nope, it doesn't, it just takes the strings passed and puts them into templates that are configured in the CROSSTOOL file (https://docs.bazel.build/versions/master/crosstool-reference.html for docs). The C++ toolchain API is really low-level, it pretty much only deals with creating the correct command line.

rust/private/rustc.bzl

rust/private/rust.bzl

hlopko

Are "fragments" going to be replaced as well, or is that a separate concept to toolchains?

Fragments are staying, they will keep serving information that is passed on the Bazel command line (such as copts and linkopts). But fragments will specifically no longer contain anything that requires reading BUILD files (up until now they could).

rust/private/rust.bzl

rust/private/rustc.bzl

hlopko · 2018-10-21T09:20:36Z

rust/private/rustc.bzl

-    cc = cpp_fragment.compiler_executable
-    ar = cpp_fragment.ar_executable
+    cc_toolchain = find_cpp_toolchain(ctx)
+    feature_configuration = cc_common.configure_features(


Hmm, I'm confused by github review, didn't I answer this already? :) cc_common is a builtin Starlark module, the list of all is here: https://docs.bazel.build/versions/master/skylark/lib/skylark-overview.html

hlopko · 2018-10-21T10:11:54Z

rust/private/rustc.bzl

+        requested_features = ctx.features,
+        unsupported_features = ctx.disabled_features,
+    )
+    ld = cc_common.get_tool_for_action(


C++ rules don't currently use platforms, they have their own thing controlled by --crosstool_top, --cpu, and --compiler (and their corresponding host variants, such as --host_crosstool_top. Here in the rule we are getting target C++ toolchain, when we're in the target configuration, and host toolchain when in host configuration (so if somebody just tells Bazel to build //foo, we build foo in target configuration. If somebody writes a genrule that has foo as a tool, Bazel will build it in host configuration). Did I answer your question?

hlopko · 2018-10-21T10:47:07Z

rust/private/rustc.bzl

+    link_variables = cc_common.create_link_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+        is_linking_dynamic_library = False,


Ideally we would ask C++ toolchain for options twice, once for executable, once for dynamic library. In practice, they usually only differ in dynamic library having --shared, which rustc might be initiative enough to add on its own. But I don't know tbh.

hlopko · 2018-10-21T10:49:50Z

rust/private/rustc.bzl

@@ -191,8 +235,6 @@ def rustc_compile_action(
    if ar_str.find("libtool", 0) != -1:
        ar = "/usr/bin/ar"


Removed. We will have to force ar and other tools to rustc when we want to be fully host-independent. Which we will have to be to support remote execution.

hlopko · 2018-10-21T10:50:36Z

rust/private/rustc.bzl

@@ -191,8 +235,6 @@ def rustc_compile_action(
    if ar_str.find("libtool", 0) != -1:
        ar = "/usr/bin/ar"


To be clear, it can (and will) happen that C++ toolchain will suggest to use libtool on darwin.

rust/private/rustc.bzl

hlopko · 2018-10-22T06:38:38Z

I'll submit now, feel free to continue discussing, I'll gladly answer or upload more fixed in a separate PR. I want to get this one in quickly so we can enable rules_rust on Bazel CI. Thanks for understanding.

PR bazelbuild/rules_rust#133 has been merged.

follow up from #133, want to see green on ci

laurentlb · 2018-12-07T16:14:23Z

I think the commit means that all users of rules_rust need to add this in their WORKSPACE file:

    http_archive(
        name = "bazel_skylib",
        url = "https://github.com/bazelbuild/bazel-skylib/archive/0.5.0.tar.gz",
        sha256 = "b5f6abe419da897b7901f90cbab08af958b97a8f3575b0d3dd062ac7ce78541f",
        strip_prefix = "bazel-skylib-0.5.0",
    )

    load("@io_bazel_rules_rust//:workspace.bzl", "bazel_version")
    bazel_version(name = "bazel_version")

PR bazelbuild/rules_rust#133 has been merged.

Migrate rules_rust to the new Starlark C++ toolchain API

aa91acc

This is needed for rules_rust to be forward compatible with Bazel 0.20. Tracking issues for migration: * bazelbuild/bazel#6380 * bazelbuild/bazel#6434 Fixes #131.

googlebot added the cla: yes label Oct 19, 2018

hlopko requested review from mfarrugi and acmcarther October 19, 2018 12:13

hlopko added the enhancement label Oct 19, 2018

hlopko self-assigned this Oct 19, 2018

hlopko added 2 commits October 19, 2018 14:53

Add check for minimum bazel version

070c37c

Run buildifier to reformat changed files

d304362

hlopko added 7 commits October 19, 2018 17:28

User skylib and make the rules backwards compatible

eb77e46

Use skylib properly

a8f56ef

Test bazel version in analysis time

d279f1b

Add workspace.bzl

1fd6cb1

Do not parse parsed bazel version

99fcd8b

Do not parse any version

3fd74d5

Do not prepend origin to rpath, toolchain api does that for us

9610f71

acmcarther approved these changes Oct 19, 2018

View reviewed changes

mfarrugi reviewed Oct 19, 2018

View reviewed changes

Polish version handling

26deb41

hlopko commented Oct 21, 2018

View reviewed changes

Merge branch 'master' into migrate_cc_toolchain_api

12e7e06

hlopko merged commit 271d790 into master Oct 22, 2018

hlopko added a commit to bazelbuild/continuous-integration that referenced this pull request Oct 22, 2018

Enable rules_rust

3aed411

PR bazelbuild/rules_rust#133 has been merged.

mfarrugi added a commit that referenced this pull request Oct 22, 2018

Enable rpaths on osx

374e505

follow up from #133, want to see green on ci

mfarrugi mentioned this pull request Oct 22, 2018

Enable dylib support on osx #142

Closed

acmcarther mentioned this pull request Dec 20, 2018

Confusion on the use of 'extra_target_triples' in rust_repository_set #109

Closed

hlopko mentioned this pull request Jan 8, 2019

incompatible_disable_legacy_cc_provider: Deprecate old C++ Starlark provider API bazelbuild/bazel#7036

Closed

acmcarther mentioned this pull request Jan 9, 2019

Bump Bazel on Travis to 0.17.1 google/cargo-raze#81

Merged

m3rcuriel mentioned this pull request Mar 13, 2019

Pass linker script through to sandbox #204

Closed

joeleba pushed a commit to joeleba/continuous-integration that referenced this pull request Jun 17, 2019

Enable rules_rust

2d26060

PR bazelbuild/rules_rust#133 has been merged.

mfarrugi deleted the migrate_cc_toolchain_api branch October 8, 2020 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate rules_rust to the new Starlark C++ toolchain API #133

Migrate rules_rust to the new Starlark C++ toolchain API #133

hlopko commented Oct 19, 2018

hlopko commented Oct 19, 2018

hlopko commented Oct 19, 2018

hlopko commented Oct 19, 2018

acmcarther left a comment

acmcarther Oct 19, 2018

hlopko Oct 21, 2018

hlopko Oct 21, 2018

acmcarther Oct 19, 2018

hlopko Oct 21, 2018

hlopko Oct 21, 2018

acmcarther Oct 19, 2018

mfarrugi Oct 20, 2018

hlopko Oct 21, 2018

hlopko Oct 21, 2018

mfarrugi left a comment

mfarrugi Oct 19, 2018

hlopko Oct 21, 2018 •

edited

Loading

hlopko Oct 21, 2018

mfarrugi Oct 22, 2018

mfarrugi Oct 19, 2018

hlopko Oct 21, 2018

hlopko left a comment

hlopko Oct 21, 2018

hlopko Oct 21, 2018

hlopko Oct 21, 2018

hlopko Oct 21, 2018

hlopko Oct 21, 2018

hlopko commented Oct 22, 2018

laurentlb commented Dec 7, 2018

		@@ -191,8 +235,6 @@ def rustc_compile_action(
		if ar_str.find("libtool", 0) != -1:
		ar = "/usr/bin/ar"

	if crate_type in ("dylib", "cdylib", "proc-macro"):
	extension = toolchain.dylib_ext

Migrate rules_rust to the new Starlark C++ toolchain API #133

Migrate rules_rust to the new Starlark C++ toolchain API #133

Conversation

hlopko commented Oct 19, 2018

hlopko commented Oct 19, 2018

hlopko commented Oct 19, 2018

hlopko commented Oct 19, 2018

acmcarther 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

mfarrugi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlopko Oct 21, 2018 • edited Loading

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

hlopko 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

hlopko commented Oct 22, 2018

laurentlb commented Dec 7, 2018

hlopko Oct 21, 2018 •

edited

Loading