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.

Sign up for GitHub

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

fix: determine the RUNFILES directory when running a js_binary in an sh_binary #286

Merged
merged 2 commits into from
Jul 14, 2022
Merged

fix: determine the RUNFILES directory when running a js_binary in an sh_binary #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
497ae41
test: test launching js_binary() rule from sh_binary()
jbedard Jul 13, 2022
ff50492
fix: determine the RUNFILES directory when running a js_binary in an …
gregmagolan Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion examples/js_binary/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ js_test(
log_level = "debug",
)

# bazel run //examples:test10_binary
# bazel run //examples:case7_binary
js_binary(
name = "case7_binary",
args = ["dummy"],
Expand Down Expand Up @@ -327,3 +327,58 @@ diff_test(
aspect_test_a_bin.bin_a_test(
name = "aspect_bin_a_test",
)

####################################################
# Use case 10
# Show launching js_binary() indirectly from a sh_binary()

write_file(
name = "write10_js",
out = "case10.js",
content = ["require('fs').writeFileSync(process.argv[2], 'case10')"],
)

js_binary(
name = "bin10-js_binary",
entry_point = "case10.js",
)

write_file(
name = "write10_launch_sh",
out = "launch_case10.sh",
content = ["""\
# --- begin runfiles.bash initialization v2 ---
set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
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 v2 ---

$(rlocation aspect_rules_js/examples/js_binary/bin10-js_binary.sh) "$@"
"""],
)

# NB: https://github.com/aspect-build/rules_js/issues/285 is only reproducable
# with 'bazel run //examples/js_binary:bin10'. The genrule below that runs
# the same binary does not reproduce as the environment is setup differently.
sh_binary(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a sh_binary and not a sh_test like we probably want. But the bug doesn't occur with sh_test. Any idea why the bug wouldn't occur when testing?

I think if we leave it as sh_binary then we need to add another rule (which I forget) to run it as a test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, it is sh_binary specific since sh_test sets the TEST_SRCDIR env var so RUNFILES is determined that way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you run the sh_binary through a genrule it is not reproducible either as the environment is setup differently in that case. only bazel run directly on the sh_binary will reproduce

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, it is sh_binary specific since sh_test sets the TEST_SRCDIR env var so RUNFILES is determined that way

Not really related to this PR... but can RUNFILES be determined the same everywhere? Doing it different for tests seems odd 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that code could get simplified in the future. Tricky to do right now as we don't have great coverage for different platforms and all of the corner cases.

name = "bin10",
srcs = [":launch_case10.sh"],
args = ["out10"],
data = [
":bin10-js_binary",
"@bazel_tools//tools/bash/runfiles",
],
)

genrule(
name = "test10",
outs = ["out10"],
# All js_binary rules need a BAZEL_BINDIR environment variable set so they can
# run from that directory as the working directory.
cmd = "BAZEL_BINDIR=$(BINDIR) $(execpath :bin10) {}/out10".format(package_name()),
tools = [":bin10"],
)
28 changes: 18 additions & 10 deletions js/private/js_binary.sh.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,12 @@ function is_windows {
# It helps to normalizes paths when running on Windows.
#
# Example:
# C:/Users/XUser/_bazel_XUser/7q7kkv32/execroot/A/b/C -> /c/users/xuser/_bazel_xuser/7q7kkv32/execroot/a/b/c
# C:/Users/XUser/_bazel_XUser/7q7kkv32/execroot/A/b/C -> /c/Users/XUser/_bazel_XUser/7q7kkv32/execroot/A/b/C
function normalize_windows_path {
# Apply the followings paths transformations to normalize paths on Windows
# -process driver letter
# -convert path separator
# -lowercase everything
sed -e 's#^\(.\):#/\L\1#' -e 's#\\#/#g' -e 's/[A-Z]/\L&/g' <<< "$1"
sed -e 's#^\(.\):#/\L\1#' -e 's#\\#/#g' <<< "$1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did windows capitalization come into play with this issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replacement of /MANIFEST is simpler if we don't lowercase it. Not sure why it was there in the first place; that code came from rules_nodejs

return
}

Expand Down Expand Up @@ -173,15 +172,24 @@ function normalize_windows_path {
if [ "${TEST_SRCDIR:-}" ]; then
# Case 4, bazel has identified runfiles for us.
RUNFILES="$TEST_SRCDIR"
elif [ "${RUNFILES_MANIFEST_ONLY:-}" ]; then
# Windows only has a manifest file instead of symlinks.
elif [ "${RUNFILES_MANIFEST_FILE:-}" ]; then
if [ "$(is_windows)" -eq "1" ]; then
# If Windows normalizing the path and case insensitive removing the `/MANIFEST` part of the path
NORMALIZED_RUNFILES_MANIFEST_FILE_PATH=$(normalize_windows_path "$RUNFILES_MANIFEST_FILE")
# shellcheck disable=SC2001
RUNFILES=$(sed 's|\/MANIFEST$||i' <<< "$NORMALIZED_RUNFILES_MANIFEST_FILE_PATH")
# If Windows, normalize the path
NORMALIZED_RUNFILES_MANIFEST_FILE=$(normalize_windows_path "$RUNFILES_MANIFEST_FILE")
else
RUNFILES=${RUNFILES_MANIFEST_FILE%/MANIFEST}
NORMALIZED_RUNFILES_MANIFEST_FILE="$RUNFILES_MANIFEST_FILE"
fi
if [[ "${NORMALIZED_RUNFILES_MANIFEST_FILE}" == *.runfiles_manifest ]]; then
# Newer versions of Bazel put the manifest besides the runfiles with the suffix .runfiles_manifest.
# For example, the runfiles directory is named my_binary.runfiles then the manifest is beside the
# runfiles directory and named my_binary.runfiles_manifest
RUNFILES=${NORMALIZED_RUNFILES_MANIFEST_FILE%_manifest}
elif [[ "${NORMALIZED_RUNFILES_MANIFEST_FILE}" == */MANIFEST ]]; then
# Older versions of Bazel put the manifest file named MANIFEST in the runfiles directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what version switched it? Can we put a comment so we can potentially remove the elif in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure and doesn't seem worthwhile to do a binary search for this one

RUNFILES=${NORMALIZED_RUNFILES_MANIFEST_FILE%/MANIFEST}
else
logf_fatal "Unexpected RUNFILES_MANIFEST_FILE value $RUNFILES_MANIFEST_FILE"
exit 1
fi
else
case "$0" in
Expand Down
28 changes: 18 additions & 10 deletions js/private/test/shellcheck_launcher.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,12 @@ function is_windows {
# It helps to normalizes paths when running on Windows.
#
# Example:
# C:/Users/XUser/_bazel_XUser/7q7kkv32/execroot/A/b/C -> /c/users/xuser/_bazel_xuser/7q7kkv32/execroot/a/b/c
# C:/Users/XUser/_bazel_XUser/7q7kkv32/execroot/A/b/C -> /c/Users/XUser/_bazel_XUser/7q7kkv32/execroot/A/b/C
function normalize_windows_path {
# Apply the followings paths transformations to normalize paths on Windows
# -process driver letter
# -convert path separator
# -lowercase everything
sed -e 's#^\(.\):#/\L\1#' -e 's#\\#/#g' -e 's/[A-Z]/\L&/g' <<< "$1"
sed -e 's#^\(.\):#/\L\1#' -e 's#\\#/#g' <<< "$1"
return
}

Expand Down Expand Up @@ -191,15 +190,24 @@ function normalize_windows_path {
if [ "${TEST_SRCDIR:-}" ]; then
# Case 4, bazel has identified runfiles for us.
RUNFILES="$TEST_SRCDIR"
elif [ "${RUNFILES_MANIFEST_ONLY:-}" ]; then
# Windows only has a manifest file instead of symlinks.
elif [ "${RUNFILES_MANIFEST_FILE:-}" ]; then
if [ "$(is_windows)" -eq "1" ]; then
# If Windows normalizing the path and case insensitive removing the `/MANIFEST` part of the path
NORMALIZED_RUNFILES_MANIFEST_FILE_PATH=$(normalize_windows_path "$RUNFILES_MANIFEST_FILE")
# shellcheck disable=SC2001
RUNFILES=$(sed 's|\/MANIFEST$||i' <<< "$NORMALIZED_RUNFILES_MANIFEST_FILE_PATH")
# If Windows, normalize the path
NORMALIZED_RUNFILES_MANIFEST_FILE=$(normalize_windows_path "$RUNFILES_MANIFEST_FILE")
else
RUNFILES=${RUNFILES_MANIFEST_FILE%/MANIFEST}
NORMALIZED_RUNFILES_MANIFEST_FILE="$RUNFILES_MANIFEST_FILE"
fi
if [[ "${NORMALIZED_RUNFILES_MANIFEST_FILE}" == *.runfiles_manifest ]]; then
# Newer versions of Bazel put the manifest besides the runfiles with the suffix .runfiles_manifest.
# For example, the runfiles directory is named my_binary.runfiles then the manifest is beside the
# runfiles directory and named my_binary.runfiles_manifest
RUNFILES=${NORMALIZED_RUNFILES_MANIFEST_FILE%_manifest}
elif [[ "${NORMALIZED_RUNFILES_MANIFEST_FILE}" == */MANIFEST ]]; then
# Older versions of Bazel put the manifest file named MANIFEST in the runfiles directory
RUNFILES=${NORMALIZED_RUNFILES_MANIFEST_FILE%/MANIFEST}
else
logf_fatal "Unexpected RUNFILES_MANIFEST_FILE value $RUNFILES_MANIFEST_FILE"
exit 1
fi
else
case "$0" in
Expand Down