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.

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

feat: add create_windows_native_launcher_script to lib/windows_utils.bzl #75

Conversation

gregmagolan
Copy link
Collaborator

Very useful for rules_js

@@ -1,3 +1,17 @@
# Copyright 2017 The Bazel Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. It is copied over verbatim from rules_nodejs. Original author was Yun Ping. A few files in this repo that were copied verbatim have this copyright.

@gregmagolan gregmagolan merged commit 0a9c48a into bazel-contrib:main Apr 14, 2022
content = r"""@echo off
SETLOCAL ENABLEEXTENSIONS
SETLOCAL ENABLEDELAYEDEXPANSION
set RUNFILES_MANIFEST_ONLY=1
Copy link

@iamricard iamricard Jul 3, 2024

Choose a reason for hiding this comment

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

👋🏼 hi and sorry for the necromancy!

This effectively makes the windows launcher not remote executable. Is there a chance you could unset this value or make it configurable (all the way up to js_binary)? We'd also need to be able to change the filepath passed as bash_bin. Alternatively, why not assume the bash file is right next to the bat and let https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash resolve correctly?

As it is, this makes js_binary (and derivatives) unusable on Windows RE setups. I don't know if this is documented/discussed in official bazel docs, but our docs on Windows RE & runfiles do cover this topic.

Copy link
Collaborator Author

@gregmagolan gregmagolan Jul 12, 2024

Choose a reason for hiding this comment

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

Hi Richard. I don't have a lot of context on this launcher code. Original author was Yun Ping from the Bazel team back in the rules_nodejs days. I'm guessing this line is here because the local Windows case doesn't have runfiles so it assumes the only runfiles manifest is present, which doesn't work on RE. Windows RE is not something we have test coverage for here or in rules_js and not our top priority at the moment, however, I'd be happy review PRs for improving this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants