Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Bash-less alternative for genrule #7503

Closed
laszlocsomor opened this issue Feb 22, 2019 · 20 comments
Closed

Windows: Bash-less alternative for genrule #7503

laszlocsomor opened this issue Feb 22, 2019 · 20 comments
Assignees
Labels
area-Windows Windows-specific issues and feature requests P1 I'll work on this now. (Assignee required) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: feature request

Comments

@laszlocsomor
Copy link
Contributor

Description of the problem / feature request:

Provide a genrule implementation (or a similar rule) that does not use Bash, but uses Cmd or PowerShell instead.

Feature requests: what underlying problem are you trying to solve with this feature?

Goal: a genrule-like rule that supports simple shell actions (e.g. copy files, run programs on inputs to produce outputs, etc.) without having to install any extra shell (i.e. should work on a clean Win10 installation).

This is part of the effort to cut Bazel's Bash dependency (#4319).

What operating system are you running Bazel on?

Windows 10

What's the output of bazel info release?

0.22.0

Have you found anything relevant by searching the web?

maprule() contains most of the necessary logic for this: https://github.com/bazelbuild/bazel-skylib/blob/master/rules/maprule.bzl

@laszlocsomor laszlocsomor self-assigned this Feb 22, 2019
@laszlocsomor laszlocsomor added type: feature request P2 We'll consider working on this in future. (Assignee optional) area-Windows Windows-specific issues and feature requests bazel 1.0 labels Feb 22, 2019
@laszlocsomor
Copy link
Contributor Author

laszlocsomor commented Feb 22, 2019

To avoid repeating the same mistake we did with Bash about assuming that certain tools are always installed with Bash (e.g. zip, grep, tar, perl), (#5265), the rule should:

  • only support cmd.exe's built-in commands (at least with the cmd_genrule variant) -- to err on the safe side, don't even assume tools like %SystemRoot%\system32\find.exe are available
  • run with an empty PATH (this also fosters reproducibility and cache-ability of artifacts)

I don't know PowerShell enough to draw are constraints about what the rule could assume is available and what isn't. Maybe someone with more expertise could chime in on this.

laszlocsomor added a commit to laszlocsomor/bazel-skylib that referenced this issue Mar 19, 2019
laszlocsomor added a commit to laszlocsomor/bazel-skylib that referenced this issue Mar 20, 2019
This rule runs a cmd.exe or Bash command.

See bazelbuild/bazel#7503
laszlocsomor added a commit to laszlocsomor/bazel-skylib that referenced this issue Mar 20, 2019
The macro select()s the 'shell' (and more)
attributes on host platform.

Example use:

    shell_actions(
        name = "foo",
        cmd_cmd = "copy /Y %SA_IN% %SA_OUT%",
        bash_cmd = "cp -f $SA_IN $SA_OUT",
        ...
    )

See bazelbuild/bazel#7503
@laurentlb
Copy link
Contributor

We can provide a simple rule in skylib:

exec_cmd(
  name = "x",
  tool = "//pkg:binary_to_run",
  args = ["-a", "-o", "$(location output_file)"],
  outputs = ["output_file", "logs"],
  stdout = "file_that_will_contain_stdout",
  stderr = "file_that_will_contain_stderr",
]

It executes a command, we can specify arguments, and we can save stdout and/or stderr to an output file. This should be able to replace many basic uses of genrule, right? And we can support this on all platforms.

@laszlocsomor
Copy link
Contributor Author

Yes, that's a great idea!

I'm also planning to add native_test and native_binary, wrapping native executables that one can bazel run and bazel test without Bash.

@laszlocsomor
Copy link
Contributor Author

@laszlocsomor
Copy link
Contributor Author

Progress!

  • native_binary (source, documentation): wraps a native binary in a *_binary rule, which you can bazel run or use in genrule.tools
  • native_test (source, documentation): wraps a native binary in a *_test rule, which you can bazel test
  • run_binary (source, documentation): runs a binary (or *_binary rule) with given inputs and expected outputs as a build action (this is a build rule wrapper for ctx.actions.run)

Still missing: some genrule-like Starlark rule that (a) needs no Bash, and (b) can run something in cmd.exe or PowerShell

@meteorcloudy
Copy link
Member

We can also consider Buck's solution:
https://buck.build/rule/genrule.html#bash

They have cmd, bash and cmd_exe attributes, when bash and cmd_exe are not provided, it fallbacks to cmd.

@laszlocsomor
Copy link
Contributor Author

laszlocsomor commented Jul 11, 2019

@meteorcloudy : Interesting idea (and nice documentation!) but do you know the reason for a common fallback attribute? Bash and Cmd syntax is so different, there's hardly any command that works exactly the same for both. (The only example I can think of is echo foo>$@, literally like so; if you added spaces or quotes you'd change the meaning.)
I think having both the Bash and Cmd command in the same rule is clever: you can depend on the same target and build with the right flavor. (It's like a select() built into the rule.)

@laszlocsomor
Copy link
Contributor Author

laszlocsomor commented Jul 11, 2019

The clear priority of bash_cmd > cmd_exe > cmd in Buck's genrule lets you build with Bash on Windows if it's installed, or fall back to Cmd.
I think that is neat, and considered doing the same, but after some thought I still wonder what's the point -- if you write the cmd_exe command, you might as well always use it on Windows. (I guess Bash might be faster (or not), but probably not by much to matter.)

@meteorcloudy
Copy link
Member

meteorcloudy commented Jul 11, 2019

How about we have only two attributes, cmd and cmd_exe (or even cmd_powershell/cmd_win?). When cmd_exe is not provided, we fallback to cmd on Windows and require Bash to be installed?
I actually have this implementation in Starlark patch API, see
https://github.com/bazelbuild/bazel/pull/8805/files#diff-e2a7f4417a4a87e4473ba69baad68652R119

@laszlocsomor
Copy link
Contributor Author

Interesting. WDYT about cmd_ps, or if not provided then fall back to cmd_exe, or if not provided then fall back to cmd_bash?

@meteorcloudy
Copy link
Member

The logic sounds ideal to me, but do we want to add cmd_bash as well? It is essentially identical to the current cmd.

@laszlocsomor
Copy link
Contributor Author

It is the same. It's just a question of naming. "cmd" might mean either "command" or "cmd.exe", so calling a bash command "cmd" next to a cmd.exe command called "cmd_exe" would be more confusing than calling the bash command "cmd_bash".

@meteorcloudy
Copy link
Member

meteorcloudy commented Jul 11, 2019

Yeah, that makes sense. I guess that's why Buck has both cmd and bash.

@meteorcloudy meteorcloudy added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jul 22, 2019
@meteorcloudy meteorcloudy self-assigned this Jul 22, 2019
@laszlocsomor
Copy link
Contributor Author

Removing "Bazel 1.0" label:

@laszlocsomor laszlocsomor added P2 We'll consider working on this in future. (Assignee optional) and removed P1 I'll work on this now. (Assignee required) bazel 1.0 labels Jul 29, 2019
@meteorcloudy
Copy link
Member

Hmm, I prefer to keep the 1.0 label. I already have a change to add cmd_bat and cmd_ps attributes to genrule, it is working but I still to add more tests. I think this is important to help our users to get away from Bash dependency on Windows.

@laszlocsomor
Copy link
Contributor Author

Oh OK!

@laszlocsomor laszlocsomor removed their assignment Jul 29, 2019
@laszlocsomor laszlocsomor added bazel 1.0 P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jul 29, 2019
@laurentlb
Copy link
Contributor

Is it really important to add them to genrule? I wonder if we can steer users away from genrule instead, and promote simpler, Stalark-based alternatives.

@meteorcloudy
Copy link
Member

I think adding them to genrule will make it easier for users to migrate away from Bash. Because

  • Users don't have to write a different rule for each genrule in order to make it work across platforms. They can just write the equivalent Batch/Powershell command and put it into a different attribute.
  • We can handle Make variable expansion in the same way as the original cmd attribute does and also we need the expanded paths to be Windows style (with forward slash). I'm not sure how well it's supported in Starlark.

Why would a Starlark based "genrule" be better than the native genrule?

@laurentlb
Copy link
Contributor

We're moving the native rules to Starlark. As genrule is complex (it also has differences between its version inside Google and outside), I wonder if we should add more features to it.

For example, Laszló wrote run_binary. This is simpler and cross-platform, although it addresses only the most simple cases.

Creating a new rule can allow us to simplify the design. We can also rename cmd to cmd_bash or something similar.

No strong feelings, go ahead if it's better for you.

@meteorcloudy
Copy link
Member

Thanks for explaining! I think the Starlark rules like run_binary is very useful under certain situations, but they still cannot replace genrule in many cases and they have a very different interface compared to genrule. So I think, in order to provide a nicer experiences for our users, I'm willing to add a bit complexity to the existing rule and take on any maintenance or migration work when nedded. :)

meteorcloudy added a commit to meteorcloudy/bazel that referenced this issue Jul 31, 2019
Extract language specific functions to an interface.

Working towards: bazelbuild#7503
meteorcloudy added a commit to meteorcloudy/bazel that referenced this issue Jul 31, 2019
meteorcloudy added a commit to meteorcloudy/bazel that referenced this issue Jul 31, 2019
bazel-io pushed a commit that referenced this issue Jul 31, 2019
Extract language specific functions to an interface.

Working towards: #7503

Closes #9022.

PiperOrigin-RevId: 260910470
meteorcloudy added a commit to meteorcloudy/bazel that referenced this issue Jul 31, 2019
bazel-io pushed a commit that referenced this issue Aug 5, 2019
Related #7503

RELNOTES[NEW]: Genrule now supports `cmd_bash`, `cmd_ps`, `cmd_bat` attributes. More details at https://docs.bazel.build/versions/master/be/general.html#genrule.cmd

PiperOrigin-RevId: 261660849
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    Extract language specific functions to an interface.

    Working towards: bazelbuild/bazel#7503

    Closes #9022.

    PiperOrigin-RevId: 260910470
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests P1 I'll work on this now. (Assignee required) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants