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

Jump to bottom

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

merged 2 commits into from
Jul 14, 2022

Conversation

gregmagolan
Copy link
Member

@gregmagolan gregmagolan commented Jul 14, 2022 •
edited
Loading

Fixes #285

Without the fix commit,

$ bazel run //examples/js_binary:bin10
INFO: Analyzed target //examples/js_binary:bin10 (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //examples/js_binary:bin10 up-to-date:
  bazel-bin/examples/js_binary/launch_case10.sh
  bazel-bin/examples/js_binary/bin10
INFO: Elapsed time: 0.139s, Critical Path: 0.00s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Build completed successfully, 2 total actions
FATAL: aspect_rules_js[js_binary]: RUNFILES environment variable is not set

with the commit

$ bazel run //examples/js_binary:bin10
INFO: Analyzed target //examples/js_binary:bin10 (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //examples/js_binary:bin10 up-to-date:
  bazel-bin/examples/js_binary/launch_case10.sh
  bazel-bin/examples/js_binary/bin10
INFO: Elapsed time: 0.191s, Critical Path: 0.00s
INFO: 2 processes: 2 internal.
INFO: Build completed successfully, 2 total actions
INFO: Build completed successfully, 2 total actions

@gregmagolan gregmagolan requested a review from jbedard July 14, 2022 01:02
jbedard
jbedard reviewed Jul 14, 2022
View reviewed changes
examples/js_binary/BUILD.bazel
"""],
)

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.

jbedard
jbedard reviewed Jul 14, 2022
View reviewed changes
js/private/js_binary.sh.tpl
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

jbedard
jbedard reviewed Jul 14, 2022
View reviewed changes
js/private/js_binary.sh.tpl
# 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

@gregmagolan gregmagolan requested review from alexeagle and jbedard July 14, 2022 01:38
@gregmagolan gregmagolan merged commit 34f9de4 into main Jul 14, 2022
@gregmagolan gregmagolan deleted the js-bin-from-sh branch February 22, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbedard jbedard jbedard approved these changes

@alexeagle alexeagle Awaiting requested review from alexeagle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

launching js_binary .sh fails when launched from sh_binary
2 participants
@gregmagolan @jbedard