bazel icon indicating copy to clipboard operation
bazel copied to clipboard

bash rlocation on windows returns unnormalized path

Open peakschris opened this issue 1 year ago • 3 comments

Description of the bug:

On windows, with --enable_runfiles, a bash script calling rlocation will receive a path containing both backslashes and forward slashes. Whether this path can be used appears down to peculiarities of the bash interpreter being used. Msys2 allows it to be read, but not used in eval.

Desired behaviour would be for rlocation to convert any backslashes in the path to forward slashes before return (unix style).

This seems to work: path1="${path1//\\//}"

Which category does this issue belong to?

External Dependency

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

create a sh_test calling:

#!/usr/bin/env bash

# --- begin runfiles.bash initialization v3 ---
# Copy-pasted from the Bazel Bash runfiles library v3.
set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash
# shellcheck disable=SC1090
source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
  source "$0.runfiles/$f" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
# --- end runfiles.bash initialization v3 ---

echo test.sh $1
if [[ -n $1 ]]; then
    echo $1 does not exist in cwd
fi
path1=$(rlocation $1)
#path1="${path1//\\//}"
echo rlocation-arg1 returns $path1
result=$(eval "$path1")
echo $result

BUILD.bazel:

sh_test(
    name = "test",
    srcs = ["test.sh"],
    args = [
        "$(rlocationpath hello.sh)",
    ],
    data = [
        "hello.sh",
        "@bazel_tools//tools/bash/runfiles",
    ],
)

D:\workdir\windows-runfiles-bug>bazel run //test2:test --enable_runfiles

Executing tests from //test2:test
-----------------------------------------------------------------------------
test.sh _main/test2/hello.sh
_main/test2/hello.sh does not exist in cwd
rlocation-arg1 returns d:\udu\b\oeelfdds\execroot\_main\bazel-out\x64_windows-fastbuild\bin\test2\test.exe.runfiles/_main/test2/hello.sh
d:\udu\b\oeelfdds\execroot\_main\bazel-out\x64_windows-fastbuild\bin\test2\test.exe.runfiles\_main\test2\test: line 22: d:uduboeelfddsexecroot_mainbazel-outx64_windows-fastbuildbintest2test.exe.runfiles/_main/test2/hello.sh: No such file or directory

Line 22 is the call to eval

Which operating system are you running Bazel on?

windows

What is the output of bazel info release?

7.2.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

peakschris avatar Jun 19 '24 11:06 peakschris

This makes sense and the fix looks simple enough. Would you be interested in sending a PR? I can review it.

fmeum avatar Jun 21 '24 12:06 fmeum

fmeum - great! I haven't signed the contributor agreement, and that's a bit complex for me. Unfortunately, I can't submit to this repo at this time. I am contributing elsewhere however.

peakschris avatar Jun 21 '24 17:06 peakschris

The path actually needs a little more fixup. I've been using this wrapper around rlocation in bash scripts that require a unix-style path:

function rlocation_as_unix() {
  path=$(rlocation ${1})
  case "$(uname -s)" in
  CYGWIN* | MINGW32* | MSYS* | MINGW*)
    path=${path//\\//} # backslashes to forward
    path=/${path//:/}  # d:/ to /d/
    ;;
  esac
  echo $path
}

assertions_sh="$(rlocation_as_unix "${assertions_sh_location}")"

peakschris avatar Jun 28 '24 23:06 peakschris

@bazel-io fork 7.4.0

fmeum avatar Sep 25 '24 12:09 fmeum

@peakschris I tried to recreate this in an integration test in #23762, but failed to reproduce the failure. Is it possible that it is mitigated by these lines in Bazel's test setup?

# Disable MSYS path conversion that converts path-looking command arguments to
# Windows paths (even if they arguments are not in fact paths).
export MSYS_NO_PATHCONV=1
export MSYS2_ARG_CONV_EXCL="*"

If that's the case, then I would definitely recommend using them in your shell scripts. I have seen many mysteriously non-portable shell scripts that were fixed by adding them.

fmeum avatar Sep 26 '24 12:09 fmeum