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

Add convenience target for running in the current working directory #29

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

mark-thm
Copy link
Contributor

@mark-thm mark-thm commented Apr 16, 2024

It's common for users of multitool to want to run tools in the current working directory. In #26, @alexeagle documented a technique we've used for a while with creating a script and symlinking to it. Our internal copy of this script is a bit more complicated to help avoid expensive calls to Bazel that simple bazel run calls don't really need. More refinements have been proposed in #27. All of these things are fundamentally workarounds for bazelbuild/bazel#3325.

To help simplify things, this PR adds a convenience wrapper that captures the execpath, switches to $BUILD_WORKING_DIRECTORY, and then runs the desired tool. The resulting shell script gets to use a very simple bazel run, should be compatible across any slew of Bazel options, and cache as well as any typical run target.

multitool/cwd.bzl Outdated Show resolved Hide resolved
Copy link

@rahul-theorem rahul-theorem left a comment

Choose a reason for hiding this comment

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

LGTM - consider the nit, but not blocking

Comment on lines -1 to -4
exports_files(
["multitool.lock.json"],
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turns out this wasn't necessary

@mark-thm mark-thm merged commit 6d0487b into main Apr 16, 2024
6 checks passed
@mark-thm mark-thm deleted the me/cwd branch April 16, 2024 01:07
@mark-thm mark-thm mentioned this pull request Apr 16, 2024
execdir="$PWD"

pushd "$BUILD_WORKING_DIRECTORY" > /dev/null
"$execdir/$tool" "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong, it leaves a bash process as the parent of the tool process, interfering with signals and leaving a messy pstree. I think you want exec here to replace the current process.
(Also I don't see the point of popd since the shell exits at that point and whatever directory it was running in is lost)

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