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

repository_ctx.execute ignores PATH env var on windows #3445

Open
snnn opened this Issue Jul 25, 2017 · 15 comments

Comments

Projects
None yet
7 participants
@snnn
Copy link
Contributor

snnn commented Jul 25, 2017

Please provide the following information. The more we know about your system and use case, the more easily and likely we can help.

Description of the problem / feature request / question:

repository_ctx.execute ignores PATH env var on windows.

There may be two "Bash"es on Windows.
"if you have bash on Ubuntu on Windows installed, you should make sure c:\tools\msys64\usr\bin appears in PATH before c:\windows\system32, because otherwise Windows' bash.exe is used before msys2's"

However, repository_ctx.execute ignores this setting.

If possible, provide a minimal example to reproduce the problem:

WORKSPACE:
load(":e.bzl", "python_configure")

python_configure(name = "local_config_python")

e.bzl:

    def test_bash(repository_ctx):
      print('start')
      print('PATH=%s' % repository_ctx.os.environ['PATH'])  
      result = repository_ctx.execute(["bash","--version"],quiet=False)
      if result.return_code == 1:
        print('error')
      print('end')
      
    python_configure = repository_rule(
        implementation = test_bash,
        environ = ["PATH"]
    )

command:
bazel build @local_config_python//:...

Output:
WARNING: D:/a/t5/e.bzl:3:3: start.
WARNING: D:/a/t5/e.bzl:4:3: PATH=C:\Tools\msys64\usr\bin;C:\jdk\bin;C:\python35;C:\python35\scripts;C:\portable\bazel;C:\usr\bin;C:\Perl64\site\bin;C:\Perl64\bin;C:\Program Files\Common Files\Microsoft Shared\Microsoft Online Services;C:\Program Files (x86)\Common Files\Microsoft Shared\Microsoft Online Services;C:\windows\system32;C:\windows;C:\windows\System32\Wbem;C:\windows\System32\WindowsPowerShell\v1.0;C:\Program Files (x86)\ATI Technologies\ATI.ACE\Core-Static;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\Git\cmd;C:\Program Files\GVFS;C:\Users\chasun\AppData\Local\Microsoft\WindowsApps;
GNU bash, version 4.3.46(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
WARNING: D:/a/t5/e.bzl:8:3: end.
ERROR: no such package '@local_config_python//': BUILD file not found on package path.
INFO: Elapsed time: 0.373s

If I run "bash --version" directly from command line, the output is:

bash --version
GNU bash, version 4.4.12(1)-release (x86_64-pc-msys)
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Environment info

  • Operating System:
    Windows 10

  • Bazel version (output of bazel info release):
    development version

  • If bazel info release returns "development version" or "(@non-git)", please tell us what source tree you compiled Bazel from; git commit hash is appreciated (git rev-parse HEAD):
    9e62187

Have you found anything relevant by searching the web?

None

Anything else, information or logs or outputs that would be helpful?

@meteorcloudy

This comment has been minimized.

Copy link
Member

meteorcloudy commented Jul 25, 2017

I can reproduce this bug.

I don't have windows bash installed on my machine, but I copied //example/cpp:hello-world binary to C:/windows, then no matter how I set my PATH, C:/Windows/bash.exe would always be found first.

I checked when executing the command, correct PATH was passed. See here
I wonder does Windows CreateProcess check some special system directories like C:/Windows before PATH? //cc @laszlocsomor

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Jul 25, 2017

From CreateProcess on MSDN [1]:

lpCommandLine [in, out, optional]

(...)

If the file name does not contain a directory path, the system searches for the executable file in the following sequence:

  1. The directory from which the application loaded.
  2. The current directory for the parent process.
  3. The 32-bit Windows system directory. Use the GetSystemDirectory function to get the path of this directory.
  4. The 16-bit Windows system directory. There is no function that obtains the path of this directory, but it is searched. The name of this directory is System.
  5. The Windows directory. Use the GetWindowsDirectory function to get the path of this directory.
  6. The directories that are listed in the PATH environment variable. Note that this function does not search the per-application path specified by the App Paths registry key. To include this per-application path in the search sequence, use the ShellExecute function.

The system adds a terminating null character to the command-line string to separate the file name from the arguments. This divides the original string into two strings for internal processing.

So apparently the Windows directory has priority above %PATH%.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx

@meteorcloudy

This comment has been minimized.

Copy link
Member

meteorcloudy commented Jul 25, 2017

@laszlocsomor Thanks! So looks like there's not much we can do from Bazel side?

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Jul 25, 2017

Indeed it does. We should wait for @dslomov 's recent patch that adds logic to search for Git Bash or MSYS Bash in the registry, then fall back to checking BAZEL_SH, but if all of those fail, then yes, unfortunately we fall back to PATH checking I suppose.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Jul 25, 2017

See 891f540

@bimmlerd

This comment has been minimized.

Copy link

bimmlerd commented Sep 20, 2017

Has the above commit made it into a release? This has just bitten us pretty hard on 0.5.4, even with BAZEL_SH set.

@meteorcloudy

This comment has been minimized.

Copy link
Member

meteorcloudy commented Sep 20, 2017

891f540 is already in 0.5.4, so it will also be in 0.6.0 (the next release, probably this week). But for some reason it's not working in 0.5.4? @dslomov

@petemounce

This comment has been minimized.

Copy link
Contributor

petemounce commented Sep 20, 2017

@bimmlerd There are RCs for 0.6.0, PTAL?

@bimmlerd

This comment has been minimized.

Copy link

bimmlerd commented Sep 21, 2017

@meteorcloudy @petemounce yep, 0.6.0-rc2 works
To be specific, we hit #3699 on 0.5.4, which looks like it might independently also get fixed in 0.6.0, but has not yet (?)

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Dec 13, 2017

This works for me on Win10 & Bazel 0.8.1:

C:\tmp\gh3445>bazel --output_user_root=c:\tmp1 build @local_config_python//:...
GNU bash, version 4.4.12(1)-release (x86_64-pc-msys)
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
DEBUG: C:/tmp/gh3445/e.bzl:2:3: start
DEBUG: C:/tmp/gh3445/e.bzl:3:3: PATH=c:/msys64/bin;c:/msys64/usr/bin;c:/work/bazel-releases/0.8.1;c:/python_27_amd64/files;c:/windows/system32
DEBUG: C:/tmp/gh3445/e.bzl:7:3: end
ERROR: Skipping '@local_config_python//:...': no such package '@local_config_python//': BUILD file not found on package path
WARNING: Target pattern parsing failed.
ERROR: no such package '@local_config_python//': BUILD file not found on package path
INFO: Elapsed time: 1.022s
FAILED: Build did NOT complete successfully (0 packages loaded)

(The error in the log is irrelevant; the presence of "end" shows it's all good.)

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Dec 13, 2017

Hold on, I need to repeat this experiment.
I recently reinstalled windows and forgot to reinstall the linux subsystem..

@meteorcloudy

This comment has been minimized.

Copy link
Member

meteorcloudy commented Dec 13, 2017

I believe the PATH environment is passed correctly, but when you run bash directly, the system checks some system directories first, so that the Windows Bash will always be found before any other bash binary in PATH.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Dec 13, 2017

nativeCreateProcess in process-jni.cc could try to resolve argv0 from the PATH before passing it to CreateProcess(...).

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Dec 13, 2017

nativeCreateProcess in process-jni.cc could try to resolve argv0 from the PATH before passing it to CreateProcess(...).

Actually, scratch that.

It should be the caller's duty to make the path absolute, not of nativeCreateProcess. The latter runs as JNI code in a DLL loaded in the Bazel server's process, thus the PATH it could read is the server's environment, not the client's (that the Bazel client passes with --client_env), and the server should ignore its own environment because the user may have changed the environment since the server launched.

So in the case of this particular bug, repository_ctx.execute should either use an absolute path, or repository_ctx should expose a .execute_shell() method to run shell commands. (That of course requires a Bash installation, which Bazel would need to find somehow, see my comment on #931.)

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

laszlocsomor commented Dec 13, 2017

...either way, this is an issue skylark/repository_ctx, so I'm adding a relevant label.
I'm keeping the Windows label because it's highly relevant for Windows, because of CreateProcess's path search semantics (see my earlier comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment