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: chdir_binary is an sh_binary that wraps a tool and does a cd first #251

Closed
wants to merge 5 commits into from

Conversation

alexeagle
Copy link
Member

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.

Copy link
Member

@gregmagolan gregmagolan left a 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):
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ptal

Copy link
Member

Choose a reason for hiding this comment

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

lgtm 🧙

gregmagolan
gregmagolan previously approved these changes Sep 20, 2022
Copy link
Member

@gregmagolan gregmagolan left a 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):
Copy link
Member

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.

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 I agree, also at the use site it's better to make this value explicit.

kormide
kormide previously approved these changes Sep 20, 2022
@alexeagle
Copy link
Member Author

@kormide WDYT about naming the new rule wrap_binary(chdir = "") so that we'd add the other features to it, rather than make more wrapper if you need to do two shimming things?

@kormide
Copy link
Member

kormide commented Sep 20, 2022

@kormide WDYT about naming the new rule wrap_binary(chdir = "") so that we'd add the other features to it, rather than make more wrapper if you need to do two shimming things?

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.

@kormide
Copy link
Member

kormide commented Sep 20, 2022

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.

@alexeagle
Copy link
Member Author

I struggled to deal with the fact that binary args and env are dropped if the tool is run under an action. I might need some help to get this across the finish line

@kormide
Copy link
Member

kormide commented Sep 21, 2022

I struggled to deal with the fact that binary args and env are dropped if the tool is run under an action. I might need some help to get this across the finish line

I can take it over.

@kormide kormide self-assigned this Sep 21, 2022
@alexeagle alexeagle dismissed stale reviews from kormide and gregmagolan September 23, 2022 01:16

reworking

@cgrindel cgrindel marked this pull request as draft October 13, 2022 17:41
@cgrindel
Copy link
Contributor

Converted to a draft until this work is prioritized.

@gregmagolan gregmagolan added the enhancement New feature or request label Feb 5, 2023
@gregmagolan gregmagolan added the wip Work-in-progress; not ready for a final review or for landing label Feb 6, 2023
@alexeagle
Copy link
Member Author

Dropping this since it's so stale and evidently not important enough that I had to come back to it.

@alexeagle alexeagle closed this May 8, 2023
@alexeagle alexeagle mentioned this pull request May 8, 2023
1 task
@alexeagle alexeagle deleted the chdir_binary branch October 20, 2023 00:04
@alexeagle alexeagle restored the chdir_binary branch October 20, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wip Work-in-progress; not ready for a final review or for landing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants