tweag / rules_sh

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

Consider consistent naming between the ruleset and the rules #1

Closed mboes closed 4 years ago

mboes commented 4 years ago

Currently, the ruleset is called rules_sh. But what is the scope of this ruleset exactly? What is currently implemented is a toolchain that exposes a set of standard "Unix" utilities, as defined by IEEE Std 1003.1-2008. But this standard is also commonly known as a particular version of POSIX (see here). So we could as well call "posix" everything we currently name "unix". This would be less confusing for Windows users, who likewise may want to use this toolchain (especially inside a MinGW shell, or inside WSL).

If we're calling the currently implemented toolchain "posix", then we might as well call the ruleset rules_posix. This would likewise be less confusing, and in keeping with the convention documented here that a rule set foo normally exposes a rules pertaining to foo as its main entry point. "sh" is a particular type of shell, ususually the Bourne shell, so is oddly specific if the intended scope of this ruleset is bigger than just exposing POSIX utilities (maybe equivalents for Microsoft shells could be considered in the future).

aherrmann commented 4 years ago

True, the toolchain called unix exposes POSIX commands and therefore posix would be a more precise name for that toolchain, I'm happy to do that.

As for the name of the ruleset, if it only exposes a posix toolchain, then rules_posix is a good name for it. However, if we end up adding support for other collections of commands then that name would be too specific. I'm worried that with a too specific name we'll just end up renaming the ruleset again later on, creating unnecessary friction.

This ruleset already supports discovering and exposing externally installed tools as a toolchain. It seems natural to extend the ruleset to expose other, maybe user-defined, collections of tools.

My reading of rules_sh is that it provides shell utilities, which describes the scope relatively well. We could call it rules_shell instead to avoid confusion with the Bourne shell.

Also looping in @laszlocsomor who suggested rules_sh in https://github.com/bazelbuild/bazel-skylib/pull/208#issuecomment-547359176.

laszlocsomor commented 4 years ago

Thanks for looping me in.

I have no preference on naming here. I suggested "rules_sh" because "sh" is the prefix for Bazel's shell rules, and I valued the consistency in naming. But that's merely my opinion.

However, let's be clear that Bazel's terminology is incorrect:

The reason for the misnomer is probably lost in time. The shell rules have always been "sh_" rules, for as long as I can remember.

mboes commented 4 years ago

Hm, simply following the precedent set by the sh_ rules (no matter how badly named), is appealing. In that case, I propose sticking with rules_sh as the name of the rule set. Should we then make the main entrypoint be sh.bzl exposing sh_* rules? @laszlocsomor points out that sh in Bazel really means "shell" in some very extensive and platform-specific sense of the word. So rules_sh ought to support multiple toolchains down the road, one for each type of shell people use sh_binary with (each of which comes with its own set of utilities called in scripts).

For consistency, I further propose to have the entrypoint for POSIX utilities be sh/posix.bzl, exposing sh_posix_configure. In the future we might have sh_cmd_configure in sh/cmd.bzl etc (or sh_win32_configure). There won't be much shared infra between these rules, so on the technical level they could as well be in separate rule sets. But since it so happens that sh_binary supports many shells, rules_sh should house them all (if we want to be consistent).

Does that sound good to everyone?

laszlocsomor commented 4 years ago

SGTM

aherrmann commented 4 years ago

Yes, sounds good. I'll make those changes.

aherrmann commented 4 years ago

The proposed changes have been implemented in https://github.com/tweag/rules_sh/pull/2. So, there doesn't seem to be anything actionable left in this issue.