Open mfarrugi opened 6 years ago
I'm in agreement with all your items. Incomplete list of my own items below (some duplication):
Repo Wide:
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:
rust.bzl
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.
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
existing_rules()
mechanism.Overall I really like their folder organization
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).
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.
One other thing to do here is crystallize / clarify the public api so that we can stop making breaking changes sooner than later.
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?
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 termskylark has changed, in particular providers, running commands (eg. run_shell), and some other apis
run_shell
struct
usages should be providers (https://docs.bazel.build/versions/master/skylark/lib/attr.html#label)[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
[x] rust_bench_test shouldn't be marked as a test
_setup_deps might not want to be producing compiler flags etc
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?