skylark updates / cleanup / refactoring

[mfarrugi]

A couple things have not kept up with changes in bazel and skylark, from what I've gathered there's:

• toolchain.bzl is somewhat misnamed since toolchain became a bazel term

  • [x] I would like to split this into rustc.bzl, toolchain.bzl, and maybe additional files. Should we split up rust.bzl some amount as well?

• skylark has changed, in particular providers, running commands (eg. run_shell), and some other apis

  • [x] rust_toolchain can require single labels/files using [attr.label(allow_single_file)]
  • [x] instances of command building should use run_shell
  • [x] struct usages should be providers (https://docs.bazel.build/versions/master/skylark/lib/attr.html#label)
  • Deprecations:
    • [x] FileType

• [x] rust.bzl might want to more carefully export an api, and/or look more like https://github.com/bazelbuild/rules_go/blob/master/go/def.bzl#L83

• [ ] mixing of % and .format strings

  • can move all to format?

• [x] rust_bench_test shouldn't be marked as a test

• _setup_deps might not want to be producing compiler flags etc

  • This has been somewhat awkward, but the codebase is small enough that moving it closer to build_rustc_command etc has felt unecessary

All in all I think these are all cosmetic, though the newer Args api might give tiny performance improvements that are pointless in the face of rustc. I'm also not excited to fix deprecations without a clearer timeline to bazel 1.0

@acmcarther is there anything else to add to the list? And what do you think is worth fixing?

[acmcarther]

I'm in agreement with all your items. Incomplete list of my own items below (some duplication):

Repo Wide:

• Probably should pick a column width limit. Not sure if buildifier has one, but I don't have a preference

known_shas.bzl: I think there is probably a better way to handle this, but I lack imagination. repositories.bzl: I recently totally reworked this. I don't have any issues with the current structure, but if toolchain.bzl gets tightened up, then this file probably should as well to match. utils.bzl: I don't have strong feelings about what is here, but I'd like to get a better sense for what should be here. triple_mappings.bzl: Needs a better name

toolchain.bzl:

• Everything unrelated to the actual toolchain decl probably should live somewhere else.
• build_*_command functions need to be reevaluated (w.r.t. changes in rust.bzl described below).

rust.bzl

• Flags to rustc need more consistent handling. I think I want them translated on site (of build_*_command or analogue) from providers.
• _setup_deps -> _setup_files
• I want more code sharing between the rust_*_impl functions
• Can giant docstrings move somewhere else? Maybe related to exported api change.

I suspect the whole implementation of rust_library itself could probably benefit from a cleanup after taking a deep dive into rules_go to see what best practices are.

---

EDIT: I'm going to do that dive now (into rules_go and others), and reply again with my learnings.

[acmcarther]

I'm still reading through rules_go, rules_scala, and rules_kotlin (the three I plan to survey for now) but I've already got some takeaways.

rules_go

• Exports their definitions and providers via a "def.bzl" file. The file is very easy to skim and has references to location in their accompanying documentation.
• Uses Providers for all steps to represent meaningful rule and inter-rule outputs.
• Uses a provider that wraps and enhances the rule ctx object with domain relevant functions such as compile, archive, etc that are backed by something they call "actions".
• Uses locally compiled go code bootstrapped using a special mode in the rules themselves to perform non-trivial operations on the machine. I suspect we wouldn't want to use Rust for the purpose here due to the need for dependencies. Perhaps Python instead.
• Packages their repository loading functionality in a concept called "sdk". I don't have a well formed opinion on this one yet, though I suppose we'll need something analogous one way or another.
• Provides a protobuf rule implementation with dependencies overridable using the existing_rules() mechanism.
• Declares constraint_value and platform for GOOS and GOARCH and config_settings to match
• Hides implementation in //go/private, rexporting the supported rules and providers in //go/def.bzl

Overall I really like their folder organization

[mfarrugi]

On moving to Args & ctx.actions.run, do you know why _setup_deps goes through the trouble of symlinking? My 2 guesses were to minimize command line length or to do easier sandboxing, neither seems particularly necessary at the moment (though I don't know if rustc suppports parameter files).

[acmcarther]

I'm reasonably confident that the symlinking is there to guarantee hermeticity. I don't think there is a good reason to make the rules less hermetic unless you have evidence that other rule implementations are doing that.

I'm pretty sure that rustc doesn't support a parameter file currently, but I remember seeing an issue floating around somewhere for it. I couldn't find it again on a cursory search though.

[mfarrugi]

One other thing to do here is crystallize / clarify the public api so that we can stop making breaking changes sooner than later.

[UebelAndre]

One other thing to do here is crystallize / clarify the public api so that we can stop making breaking changes sooner than later.

Does https://github.com/bazelbuild/rules_rust/blob/main/ARCHITECTURE.md satisfy this? Can this issue be closed?