Use Bazel's COMPILATION_MODE var to determine opt-level and debuginfo… #97

vitalyd · 2018-06-11T21:33:08Z

… codegen options

This change looks at Bazel's COMPILATION_MODE var, surfaced on the CLI as -c [dbg|fastbuild|opt], and adjusts the opt-level and debuginfo codegen options passed to rustc. I tried to mirror what cargo does in its dev and release build profiles.

If there's a better way (I'm fairly new to bazel and bazel rust), I'm all ears.

… codegen options This change looks at Bazel's `COMPILATION_MODE` var, surfaced on the CLI as `-c [dbg|fastbuild|opt]`, and adjusts the `opt-level` and `debuginfo` codegen options passed to `rustc`. I tried to mirror what `cargo` does in its `dev` and `release` build profiles. If there's a better way (I'm fairly new to `bazel` and `bazel rust`), I'm all ears.

googlebot · 2018-06-11T21:33:11Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

mfarrugi · 2018-06-11T21:43:58Z

lgtm (sans cla), any thoughts @acmcarther?

acmcarther

This generally looks fine to me, one comment.

acmcarther · 2018-06-11T22:31:01Z

rust/toolchain.bzl

+
+  # TODO consider handling 'fastbuild'
+  # opt_level and debug_info combination roughly matches Cargo's "dev" and "release" profiles
+  (opt_level, debug_info) = (0, 2) if ctx.var["COMPILATION_MODE"] == "dbg" else (3, 0)


Do you have an aversion to adding support for fastbuild in this PR? Best way to do that is probably to pull this into a separate function, with the function returning a struct containing opt_level and debug_info. (0, 0) is probably most appropriate for fastbuild.

With that change, it would probably be appropriate to drop the "@todo" from the opt-level line as well.

@acmcarther, the problem with (0,0) for fastbuild is that fastbuild is the default mode. This means we'll get a rather useless binary/lib: no debug info and no optimizations. This is essentially why I punted on "truly" supporting fastbuild - I think it requires some thought, unless you think it being the default isn't really an issue and users should explicitly opt in for opt builds when they want performance or dbg if they want full debug info.

For convenience, here is the description of the 3 modes in bazel's user manual. debuginfo=0, opt-level=0 is in the spirit of fastbuild, but my main concern is a change of behavior for existing users that aren't explicitly specifying this option, which admittedly, seems quite C/C++ oriented (as its description even mentions). That's also the reason I wasn't sure using -c was appropriate for non-C/C++ builds, even if they can mirror the debug info/opt level settings.

For my uses, I'm fine with explicitly specifying -c opt or -c dbg. In cargo land, fastbuild is also somewhat close to cargo check, which is essentially "run everything up to LLMV codegen". It doesn't produce an artifact, however, but is super quick :). Although we likely can't use that for fastbuild.

my main concern is a change of behavior for existing users that aren't explicitly specifying this option, which admittedly, seems quite C/C++ oriented (as its description even mentions).

To be honest, I'm personally not overly concerned about changing the default optimization level. There have been some discussions about correctly supporting -c (opt|dbg|fastbuild) elsewhere, and I think the expectation is that they should work as they do in C++. If a user wants to have a different optimization level per compilation target, they can do so via rustc_flags. By default, I do specify an optimization level in cargo-raze, so I expect that this will be a no-op for many build targets.

In cargo land, fastbuild is also somewhat close to cargo check, which is essentially "run everything up to LLMV codegen". It doesn't produce an artifact, however, but is super quick :). Although we likely can't use that for fastbuild.

I don't particularly care for cargo check as a basis to work forward from as far as rust_library itself is concerned but if people are interested in this functionality, I think that Bazel Aspects are a good fit. They can traverse the build tree and convert rust_library invocations into those no-output-y check-like invocations

All that said, take those perspectives with a grain of salt. I'm the resident Cargo/"existing rust practice" antagonist though, so /cc @mfarrugi and @damienmg for more well developed opinions about changing the default optimization level.

I tend to default to dbg mode in my setup. I think fastbuild is not correct but is kept because in Google, binary tend to be very big so fastbuild is a better compromise.

Also to be consistent I would just do the fastbuild do a fast build and expect people to have -c dbg on their workstation. Also probably having fastbuild on a CI is a good thing

mfarrugi · 2018-06-12T16:37:37Z

Our build would need #86 to revert a change to the default behavior, because we control this in a bazelrc that is building with fastbuild and --copts. From that perspective, I don't mind the half measure. (No thoughts on cargo check yet)

…

On Tue, Jun 12, 2018, 12:18 Alex McArther ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In rust/toolchain.bzl <#97 (comment)>: > @@ -52,6 +52,10 @@ def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir, extra_filename = "" if output_hash: extra_filename = "-%s" % output_hash + + # TODO consider handling 'fastbuild' + # opt_level and debug_info combination roughly matches Cargo's "dev" and "release" profiles + (opt_level, debug_info) = (0, 2) if ctx.var["COMPILATION_MODE"] == "dbg" else (3, 0) my main concern is a change of behavior for existing users that aren't explicitly specifying this option, which admittedly, seems quite C/C++ oriented (as its description even mentions). To be honest, I'm personally not overly concerned about changing the default optimization level. There have been some discussions about correctly supporting -c (opt|dbg|fastbuild) elsewhere, and I think the expectation is that they should work as they do in C++. If a user wants to have a different optimization level per compilation target, they can do so via rustc_flags. By default, I do specify an optimization level in cargo-raze, so I expect that this will be a no-op for many build targets. In cargo land, fastbuild is also somewhat close to cargo check, which is essentially "run everything up to LLMV codegen". It doesn't produce an artifact, however, but is super quick :). Although we likely can't use that for fastbuild. I don't particularly care for cargo check as a basis to work forward from as far as rust_library itself is concerned but if people are interested in this functionality, I think that Bazel Aspects <https://docs.bazel.build/versions/master/skylark/aspects.html> are a good fit. They can traverse the build tree and convert rust_library invocations into those no-output-y check-like invocations ------------------------------ All that said, take those perspectives with a grain of salt. I'm the resident Cargo/"existing rust practice" antagonist though, so /cc @mfarrugi <https://github.com/mfarrugi> and @damienmg <https://github.com/damienmg> for more well developed opinions about changing the default optimization level. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLjeKEeCC2kxgGE_r16pn7E1zUrj9zZks5t7-nEgaJpZM4Ujbq6> .

vitalyd · 2018-06-12T17:06:08Z

I've signed the CLA in the meantime.

vitalyd · 2018-06-12T19:34:44Z

@acmcarther, I've made the suggested changes.

vitalyd · 2018-06-13T17:50:03Z

@acmcarther / @mfarrugi, do you think this is good to go?

mfarrugi · 2018-06-14T13:57:28Z

Ci is unhappy and I don't think I can fix either check. @acmcarther?

acmcarther · 2018-06-14T15:53:34Z

I just unblocked the CI and it passed, sans CLA.

@vitalyd: I'm not sure what the CLA bot uses to trigger a CLA re-evaluation, can you say I signed it! verbatim here?

Also check https://cla.developers.google.com/clas to double check that status.

vitalyd · 2018-06-14T17:38:15Z

I signed it!

vitalyd · 2018-06-14T17:41:03Z

@acmcarther, I looked at that cla link and it all seems good. I tried a verbatim reply above - let's see if that nudges it. The instructions above said:

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

It wasn't clear whether it had to be that text verbatim or that was just an example (and hence the "e.g."). But I guess we'll find out soon enough 😄

damienmg · 2018-06-14T21:57:58Z

@acmcarther you can check people's CLA on signcla/
@vitalyd You do not appear neither with your github account nor with your username in the database. You might have registered with another email than the one appearing on the commit? Also don't forget to put your github account on the form.

damienmg · 2018-06-14T22:00:10Z

Also about the content:
In C++ the flag set depending on the compilation mode is something set in the crosstool which the legacy version of the toolchain rule, this would probably make sense to have the debug level and the opt level depending on the compilation mode be a dictionary that you define through the toolchain. You can give default value as the current one so it will be in practice the same but the user would have possibility to override by specifying its own toolchain.

vitalyd · 2018-06-15T00:43:12Z

@vitalyd You do not appear neither with your github account nor with your username in the database. You might have registered with another email than the one appearing on the commit? Also don't forget to put your github account on the form

@damienmg, thanks for the pointer! I'm (embarrassingly) late to the github game, and I indeed only signed the agreement with my corp account. I've now signed it with my personal account.

googlebot · 2018-06-15T00:43:15Z

CLAs look good, thanks!

vitalyd · 2018-06-15T00:47:03Z

In C++ the flag set depending on the compilation mode is something set in the crosstool which the legacy version of the toolchain rule, this would probably make sense to have the debug level and the opt level depending on the compilation mode be a dictionary that you define through the toolchain. You can give default value as the current one so it will be in practice the same but the user would have possibility to override by specifying its own toolchain.

@damienmg, so I'm quite new to Bazel. Are you suggesting that C++'s CROSSTOOL configuration is the model to follow or that it's "legacy" and should not be followed?

I like your suggestion of making it a toolchain configuration, though - is there an example I can look at if it's not the C++ toolchain? Want to see how this is defined and then wired up at bazel build time. Thanks

damienmg · 2018-06-16T15:29:27Z

rust/toolchain.bzl

@@ -68,7 +91,8 @@ def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
          src.path,
          "--crate-name %s" % crate_name,
          "--crate-type %s" % crate_type,
-          "--codegen opt-level=3",  # @TODO Might not want to do -o3 on tests
+          "--codegen opt-level=%s" % codegen_opts.opt_level,  # @TODO Might not want to do -o3 on tests


Nit: remove todo

damienmg · 2018-06-16T15:38:24Z

The CROSSTOOL file is the legacy version of the toolchain rules and basically toolchain where develop with the C++ CROSSTOOL example in mind. Which means that if C++ where to use the toolchain rules, they would store the configuration that depends on the compilation mode a toolchain attribute :)

I don't think such example exists, rules_go hard-code the flags in their code. Anyway that could looks like (with my PR to add the target triplet in it) :

def _get_comp_mode_codegen_opts(ctx, toolchain):
  mode =  ctx.var["COMPILATION_MODE"]
  if not mode in toolchain.compilation_mode_opts:
    fail("Unrecognized compilation mode %s for toolchain for triplet %s, available modes are %s" % (mode, toolchain.triplet, compilation_mode_opts.keys()))
  return toolchains.compilation_mode_opts[mode]
...
  codegen_opts = _get_comp_mode_codegen_opts(ctx, toolchain)
...
def _rust_toolchain_impl(ctx):
   compilation_mode_opts = {}
   for k, v in ctx.attr.opt_level.items():
     if not k in ctx.attr.debug_info:
       fail("Compilation mode %s is not defined in debug_info but is defined opt_level" % k)
     compilation_mode_opts[k] = struct(opt_level = v, debug_info = ctx.attr.debug_info[k])
   for k, v in ctx.attr.debug_info.items():
     if not k in ctx.attr.opt_level:
       fail("Compilation mode %s is not defined in opt_level but is defined debug_info" % k)
   toolchain = platform_common.ToolchainInfo(
       ...
       compilation_mode_opts = compilation_mode_opts,
       ...
       crosstool_files = ctx.files._crosstool)
  return [toolchain]

rust_toolchain = rule(
     _rust_toolchain_impl,
     attrs = {
     ...
     opt_level = attr.string_dict(default = {"opt": "3", "dbg": "0", "fastbuild": "0"}),
     debug_info =  attr.string_dict(default = {"opt": "0", "dbg": "2", "fastbuild": "0"}),
     ...

vitalyd · 2018-06-18T13:28:50Z

rust/toolchain.bzl

+def _get_comp_mode_codegen_opts(ctx, toolchain):
+  comp_mode = ctx.var["COMPILATION_MODE"]
+  if not comp_mode in toolchain.compilation_mode_opts:
+    fail("Unrecognized compilation mode %s for toolchain." % comp_mode)


@damienmg, I didn't see your target triple changes in so leaving that out of the message (for now).

vitalyd · 2018-06-18T13:31:08Z

@damienmg, I applied your suggestion of making the two settings toolchain options.

Tangentially, do you know how to query bazel to see what it would set opt_level and debug_info to without doing an actual build? I know I can run a build with /s to get verbose output, but I'd like to know a way to query toolchain options selected with, say, a particular COMPILATION_MODE without building a target.

damienmg · 2018-06-18T13:45:27Z

I don't know any way to do that because you want the action manifest. cquery can tell you which toolchain is selected bazel cquery -c opt 'kind(rust_toolchain, deps(@examples//fibonacci:fibonacci_test))' but that won't tell you how the action is generate from there.

vitalyd · 2018-06-18T14:57:48Z

I don't know any way to do that because you want the action manifest. cquery can tell you which toolchain is selected bazel cquery -c opt 'kind(rust_toolchain, deps(@examples//fibonacci:fibonacci_test))' but that won't tell you how the action is generate from there.

That's too bad, but ok.

So, are we good on this change or is there anything else I should do? cc @acmcarther

acmcarther

I don't really feel strongly about these suggestions, and I know you're getting ping ponged between my comments and @damienmg's suggestion.

I'm OK with what we have if you'd just like to get it submitted.

acmcarther · 2018-06-18T20:50:13Z

rust/toolchain.bzl

@@ -221,6 +241,8 @@ rust_toolchain = rule(
        "_crosstool": attr.label(
            default = Label("//tools/defaults:crosstool"),
        ),
+        "opt_level": attr.string_dict(default = {"opt": "3", "dbg": "0", "fastbuild": "0"}),


Optional Nit:

I find the interchanging use of rust's opt and dbg and bazel's "opt" and "db" to be very confusing here.

Are you open to making this more verbose: "compilation_mode_to_opt_level" and "compilation_mode_to_debug_info_level"?

How about naming the attributes rust_opt_level and rust_debug_info, respectively? I'm not opposed to the verbose names you suggest, but I'm also not bothered by the current scheme either :).

I come from a Java context, so I don't feel sufficiently spoon fed when rust_opt_level contains something that isn't a string or num. The map implies some kind of relationship or nuance that is hidden from the attr name.

It seems marginally better to my eyes though, so if status quo is the way to go, then rust_ SGTM.

IMHO, unless someone presents a slam dunk option, I’m inclined to leave this as-is and save the bikeshedding for a future iteration, perhaps when we expand the list of codegen options we support.

Otherwise, we’re iterating on names none of which seem significantly better than the others.

acmcarther · 2018-06-18T20:56:20Z

rust/toolchain.bzl

@@ -197,6 +207,15 @@ def _out_dir_setup_cmd(out_dir_tar):
 # The rust_toolchain rule definition and implementation.

 def _rust_toolchain_impl(ctx):
+  compilation_mode_opts = {}
+  for k, v in ctx.attr.opt_level.items():


Optional Nit:

I think I personally prefer just using the known variants of COMPILATION_MODE, rather than letting this be freeform. That is to say, just iterate over ["opt", "dbg", "fastbuild"], and disregard other values.

That is to say, I think that the toolchain parameters probably have too much flexibility.

vitalyd · 2018-06-19T11:40:13Z

@acmcarther / @damienmg, as mentioned, I'm inclined to push the latest set of changes in this PR. Unless you guys have something that you insist on changing, I don't think we're making any forward progress with the name changes. Let me know what you think.

mfarrugi · 2018-06-19T14:00:40Z

Looks like we've been at a mergeable spot for a while, can address any remaining bikeshedding separately.

It's worth noting that these variable names are part of the public API though, and at some point they may be painful to change.

googlebot added the cla: no label Jun 11, 2018

acmcarther reviewed Jun 11, 2018

View reviewed changes

Explicitly handle fastbuild compilation mode.

c7973e6

googlebot added cla: yes and removed cla: no labels Jun 15, 2018

damienmg reviewed Jun 16, 2018

View reviewed changes

Make opt_level and debug_info toolchain options

0341568

vitalyd commented Jun 18, 2018

View reviewed changes

acmcarther reviewed Jun 18, 2018

View reviewed changes

mfarrugi merged commit 1a05fcc into bazelbuild:master Jun 19, 2018

Use Bazel's COMPILATION_MODE var to determine opt-level and debuginfo… #97

Use Bazel's COMPILATION_MODE var to determine opt-level and debuginfo… #97

Conversation

vitalyd commented Jun 11, 2018

googlebot commented Jun 11, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

mfarrugi commented Jun 11, 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

mfarrugi commented Jun 12, 2018 via email

vitalyd commented Jun 12, 2018

vitalyd commented Jun 12, 2018

vitalyd commented Jun 13, 2018

mfarrugi commented Jun 14, 2018

acmcarther commented Jun 14, 2018

vitalyd commented Jun 14, 2018

vitalyd commented Jun 14, 2018 • edited Loading

damienmg commented Jun 14, 2018

damienmg commented Jun 14, 2018

vitalyd commented Jun 15, 2018

googlebot commented Jun 15, 2018

vitalyd commented Jun 15, 2018

Choose a reason for hiding this comment

damienmg commented Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

vitalyd commented Jun 18, 2018

damienmg commented Jun 18, 2018

vitalyd commented Jun 18, 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

vitalyd commented Jun 19, 2018

mfarrugi commented Jun 19, 2018

vitalyd commented Jun 14, 2018 •

edited

Loading

damienmg commented Jun 16, 2018 •

edited

Loading