-
-
Notifications
You must be signed in to change notification settings - Fork 99
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: chdir_binary is an sh_binary that wraps a tool and does a cd first #251
Conversation
5e35095
to
25d2a30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Could use a test but otherwise lgtm
lib/paths.bzl
Outdated
@@ -22,3 +23,44 @@ source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/ | |||
{ echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e | |||
# --- end runfiles.bash initialization v2 --- | |||
""" | |||
|
|||
def chdir_binary(name, binary, chdir = "$BUILD_WORKSPACE_DIRECTORY", **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this in chdir_binary.bzl
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a new public API entrypoint, or in private/chdir_binary.bzl and exposed in paths.bzl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer a new public API since paths is currently just small utility functions. more entry points the better for bazel-lib IMO since it is a utility library and it makes the docs easier to navigate on the left navbar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ptal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🧙
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌮
load(":utils.bzl", "to_label") | ||
load("@bazel_skylib//rules:write_file.bzl", "write_file") | ||
|
||
def chdir_binary(name, binary, chdir = "$BUILD_WORKSPACE_DIRECTORY", **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm wondering if the BUILD_WORKSPACE_DIRECTORY
is intuitive. Most Bazel users are accustomed to binaries running either in the execroot or the runfiles root. I feel like defaulting to the source tree could confuse some people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree, also at the use site it's better to make this value explicit.
@kormide WDYT about naming the new rule |
Yeah not having to wrap a binary several times sounds like a better DX. So long as most things we need to wrap can be accomplished by just adding one attribute to the macro and they are generally mutually exclusive. |
The more I think about it, the more I can see how an "everything" macro could get out of hand. I think I'm going to change my opinion here and vote for small functions. Users can macroize several of them together if they need to. |
I struggled to deal with the fact that binary |
I can take it over. |
Converted to a draft until this work is prioritized. |
Dropping this since it's so stale and evidently not important enough that I had to come back to it. |
As far as I know, java_binary still can't be directly
bazel run
so that it starts in the working directory, and Java makes it nearly impossible to change the working directory of the JVM. @shs96c can correct me I'm sure.