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: don't shell out to cmd.exe #2190

Closed
laszlocsomor opened this issue Dec 6, 2016 · 16 comments
Closed

Windows: don't shell out to cmd.exe #2190

laszlocsomor opened this issue Dec 6, 2016 · 16 comments
Labels
area-Windows Windows-specific issues and feature requests P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) platform: windows team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug
Milestone

Comments

@laszlocsomor
Copy link
Contributor

Description of the problem / feature request / question:

cmd.exe doesn't support UNC paths so we can't shell out to tools through it and use long paths.

Example where this occurs: WindowsFileSystem.createDirectoryJunction

Related issue: #2181

Environment info

  • Operating System: Windows 10
@laszlocsomor laszlocsomor added platform: windows P1 I'll work on this now. (Assignee required) type: feature request labels Dec 6, 2016
@laszlocsomor
Copy link
Contributor Author

This issue is actually blocking #2181 .

@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) labels Dec 6, 2016
@laszlocsomor
Copy link
Contributor Author

P2, because there is a workaround: use --output_user_root=<something short>.

You can even create a virtual drive:

subst d: c:\some\path\where\bazel\can\put\stuff

then just use --output_user_root=/d/.

@laszlocsomor
Copy link
Contributor Author

Re: #2190 (comment) and WindowsFileSystem.createDirectoryJunction, that's actually a bad example, because mklink is a built-in command of cmd.exe, so we'll have to implement the junction creation in JNI anyway.

The CommandBuilder however suffers from this limitation.

@abergmeier-dsfishlabs
Copy link
Contributor

Isn't this rather a bug?

@dslomov dslomov added this to the 0.5 milestone Dec 6, 2016
@petemounce
Copy link
Contributor

Generally don't use cmd, because MS announced that they will (finally!!) be deprecating it (over some time): http://www.osnews.com/story/29502/Microsoft_replaces_cmd_with_PowerShell_in_new_Windows_10_build

@dslomov dslomov modified the milestones: 0.6, 0.5 Feb 14, 2017
@laszlocsomor
Copy link
Contributor Author

laszlocsomor commented Feb 14, 2017

@petemounce : AIUI they aren't deprecating cmd, just making it non-default. So cmd will be around... *in ghastly voice* foreveeeeeeer.

@petemounce
Copy link
Contributor

Sure. Still don't use it. :p

@meteorcloudy meteorcloudy added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels Feb 28, 2017
@dslomov dslomov modified the milestones: 0.6, 0.5 Apr 4, 2017
@dslomov
Copy link
Contributor

dslomov commented Jul 24, 2017

Powershell is very very slow to start (as in, >10x slower than cmd.exe as @philwo measured)
So we avoid using powershell as a shell.
See https://docs.google.com/document/d/1z6Xv95CJYNYNYylcRklA6xBeesNLc54dqXfri0z0e14/ for some plans

@dslomov dslomov modified the milestones: 0.7, 0.6 Jul 24, 2017
@davido
Copy link
Contributor

davido commented Aug 19, 2017

Another reason to not shell out to cmd.exe is 8191 command line limit: #3579.

@laszlocsomor
Copy link
Contributor Author

This is the only non-test place I found where Bazel shells out to cmd.exe:

return new String[] { "CMD.EXE", "/S", "/E:ON", "/V:ON", "/D", "/C",

I have a hunch (that I need to verify) that you can CreateProcess with other executables than .exe files, so automatically using cmd may be unnecessary:

// Automatically enable CMD.EXE use if we are executing something else besides "*.exe" file.
// When use CMD.EXE to invoke a bat/cmd file, the file path must have '\' instead of '/'

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 12, 2018
See bazelbuild#2190

Change-Id: If665af264a23be0219c75ae087dd25db74d5e386
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 12, 2018
See bazelbuild#2190

Change-Id: If665af264a23be0219c75ae087dd25db74d5e386
bazel-io pushed a commit that referenced this issue Apr 12, 2018
See #2190

Closes #5005.

Change-Id: If665af264a23be0219c75ae087dd25db74d5e386
PiperOrigin-RevId: 192575414
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 12, 2018
Remove the .useShell method, expect callers to
just pass the shell interpreter if they need it.
This removes the argument vector transformation
heuristic, and stops shelling out to cmd.exe on
Windows. See bazelbuild#2190

Also remove the .setWorkingDir method because
callers always had to set the working directory.
Instead, the CommandBuilder constructor takes the
working directory.

Change-Id: I545e01c811daaf34913cb585492923da81aa02ee
@laszlocsomor laszlocsomor reopened this Apr 17, 2018
@laszlocsomor
Copy link
Contributor Author

PR #5007 was rolled back in 8358148.

@laszlocsomor
Copy link
Contributor Author

Apparently the CommandBuilder.java can still shell out to cmd.exe, so this bug is not fixed.

@laszlocsomor
Copy link
Contributor Author

Other than CommandBuilder.java, the Android tools' junction helper (that I wrote) also uses cmd:

"cmd.exe /C mklink /J \"%s\" \"\\\\?\\%s\"" % (name, target),

These are the only two locations I'm aware of. (And some more in Bazel's own tests.)

@laszlocsomor
Copy link
Contributor Author

I'm not sure if this bug is really that big of a deal:

  • For the Android tools: mklink supports long paths, even when invoked from cmd. The limiting factor for the path length of the junction target is the 8K command line limit.
  • Bazel shells out to cmd.exe only when:
    • the action sets /bin/sh or /bin/bash as its executable (I could not find any native rules doing so; I can't rule out Starlark rules doing it via the deprecated ctx.action()), or
    • when the argv0 does not end with ".exe", which should only happen on Windows if the extension is ".bat" or ".cmd" (very rare) which are fine to run under cmd.exe

@laszlocsomor
Copy link
Contributor Author

Because of #2190 (comment), I'm marking it as P4.

@laszlocsomor laszlocsomor added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) bazel 1.0 labels Mar 1, 2019
@meisterT
Copy link
Member

Closing for the reasons mentioned above.

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
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 P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) platform: windows team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug
Projects
None yet
Development

No branches or pull requests

10 participants