search

tweag / rules_sh

Shell rules for Bazel
Apache License 2.0
42 stars 3 forks source link

Consider reducing the number of attributes in or sharding sh_posix_toolchain #13

Closed michajlo closed 4 years ago

michajlo commented 4 years ago

See also https://github.com/bazelbuild/bazel/issues/10953

sh_posix_toolchain has 180 attributes, which wound up breaking Bazel's attempt to cap the number of attributes per rule-class at (what we thought was a generous) 150: https://github.com/bazelbuild/bazel/commit/b839a51d346b61815fd48de2396250c80d1713f0. It was easy enough to raise the limit to accommodate sh_posix_toolchain in this case, but it won't always be that way due to misc implementation details.

Please consider if this rule really needs so many attributes. Capping growth will ensure this rule's continued viability. Reducing attributes will help reduce outliers, which helps us better optimize for the common case.

aherrmann commented 4 years ago

sh_posix_toolchain has one attribute per available command https://github.com/tweag/rules_sh/blob/0c274ad480ed3eade49250abd04ff71655a07820/sh/posix.bzl#L13-L14

It should be possible to change this to a single string_dict attribute instead.

smelc commented 4 years ago

rules_nixpkgs's nixpkgs.bzl must be updated accordingly when this change is performed, as it depends on sh_posix_toolchain prototype: https://github.com/tweag/rules_nixpkgs/blob/451876c1521612409d83865166cfdcb5dc7fcb0c/nixpkgs/nixpkgs.bzl#L507-L511