regression: genrules leaking PATH into environment

[msolo-dropbox]

PATH=/home/vagrant/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/go/bin:/srv/virtualenv/lint/bin
PWD=/home/vagrant/.cache/bazel/_bazel_vagrant/bfffec75e84be56b2e7ae2032dda086a/server
SHLVL=1
_=/usr/bin/printenv
ERROR: /home/vagrant/src/server/thirdparty/esde/BUILD:9:1: Executing genrule //thirdparty/esde:_esde failed: bash failed: error executing command
(cd /home/vagrant/.cache/bazel/_bazel_vagrant/bfffec75e84be56b2e7ae2032dda086a/server &&
exec env -
PATH=/home/vagrant/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/go/bin
/bin/bash -c 'source external/bazel_tools/tools/genrule/genrule-setup.sh; printenv |sort; exit 1; '): com.google.devtools.build.lib.shell.BadExitStatusException: Process exited with status 1: bash failed: error executing command

This is clearly a regression - this was not happening in previous releases.

[laszlocsomor]

Could you say more about what the problem is? What's the expected behavior, what should PATH contain? How do we repro? When did this use to work / behave differently?

[msolo-dropbox]

My previous release was some months old, from July. At that time, the path was "/usr/bin:/bin" - anything else had to be explicity referenced with an absolute path or placed in the evnironment.

The "exec env -" shows the you are trying to clear the environment, but then you place a leaky definition of PATH coming from the operator's console. That seems like a problem, particularly in standalone mode.

This genrule should illustrate the issue:

genrule(                           
  name = 'fail',                   
  srcs = ['.'],                    
  outs = [                         
    'fail.txt',                    
    ],                             
  cmd = 'printenv | sort; exit 1', 
)                                  

[laszlocsomor]

Thanks! That was really helpful. I could repro the problem. Looking into it.

[laszlocsomor]

This is the culprit: 946a2fb

I'll assign the bug to Damien who submitted the change, he should know more about why he changed it.

[damienmg]

This a 4 month old change I don't even remember :) I'll take a look at it
tomorrow.

On Thu, Nov 12, 2015, 5:56 PM László Csomor notifications@github.com
wrote:

This is the culprit: 946a2fb
946a2fb

I'll assign the bug to Damien who submitted the change, he should know
more about why he changed it.

—
Reply to this email directly or view it on GitHub
#577 (comment).

[damienmg]

AFAICT this is WAI. The previous behaviour was problematic.

In skylark action we have use_default_shell_env that controls that. Maybe we should add something similar in genrule?

[laszlocsomor]

Damien, AIUI the OP is about genrule leaking the user's PATH into the action. That violates hermeticity, doesn't it? Could you say more about why this is WAI?

[damienmg]

With Bazel we cannot determine which PATH are standard on the target platform so it use the env variable by default when use_default_shell_env is on. Without that flag the path is ':' which is wrong.

This is WAI but not necessarly as desired :)

[laszlocsomor]

Could we use the system's default PATH?

Compare your own results for:

$ exec env - /bin/bash -c 'echo "hello path=($PATH)"'

and

$ exec /bin/bash -c 'echo "hello path=($PATH)"'

The first one prints the system's default PATH, the second one prints my own PATH.

[laszlocsomor]

If I comment out this line [1], build my bazel, then use that to build this genrule [2], then the output is this the following:

$ /tmp/bazel/bazel-bin/src/bazel build --package_path=%workspace%:/tmp/bazel 2>/dev/null //foo:fail
hello path=(/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin:.)
PWD=(...)
SHLVL=1
_=/usr/bin/printenv

I'm surprised though that that printenv didn't print the PATH even though it clearly has a value.

[1] https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java#L71

[2]

# //foo/BUILD
genrule(
    name = "fail",
    outs = ["fail.txt"],
    cmd = "echo \"hello path=($$PATH)\" ; printenv | sort; exit 1",
)