-
Notifications
You must be signed in to change notification settings - Fork 108
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
Allow a custom bazel binary name (or path) #191
base: main
Are you sure you want to change the base?
Conversation
Hi, Ollie! Great to meet you. Just seeing this and your issue--sorry a little backed up over here. Thanks for being the kind of person who aims to leave things better than you found them! I'm certainly open to this in spirit, but I'm pretty sure I recall the last person who raised this finding that the Bazel idiomatic way of doing this was to use some built-in magic Bazel functionality for wrapper scripts, rather than configure by tool. Maybe it was to put the wrapper in tools/bazel? Is that something you've already tried? Cheers--and happy coding! |
Thanks for getting back to me! |
oh i didn't check for this and filed something similar #194 in our case we don't have a |
IMO this PR is better than mine since you can set it on the rule |
@cpsauer what is you stance on merging this? Missing anything or should we begin to patch our local installation? |
Hey Ollie--stance is positive and want to support rather than making you maintain a patch. Just got backlogged. Sorry :/ Do want to make sure there's not a better way--e.g. you guys are pretty sure you don't want this as an environment variable, right? This seems like something you'd want inherited by all refresh_compile_command rules, even ones already defined, like :refresh_all, and this seems more like a per-setup kind of thing than a per-rule or per-repo. I'd propose just using PATH to add your wrapper script, Ollie, but @keith, it sounds like that doesn't work for you? Anything more I should know there about the hermiticity concerns there? I presume you're passing --incompatible_strict_action_env, and that it doesn't work to put it on your path just for these calls, like Edit: Ollie, might the bazel idiomatic way of doing this be to use bazelisk to get bazel and then tools/bazel to get the rest? |
Yea for sure in our case it wouldn't be in the PATH. It's just a wrapper binary at the root of the repo that makes the UX better by not requiring users to have any bazel install ahead of time, or think about version issues. |
oh I just made a PR for the same issue: #215 we had to make it a --define though since we actually change wrapper scripts on the command line so we need to be able to override the wrapper script to use in the subshells also via the command line, and I couldn't figure out how to override it when I did it the way that it is done here. |
@sthornington I see. Will read your PR. Are you not overriding the binary when setting up the refresh-compile-commands rule? |
Nevertheless, we have had this PR as patch working at my company for some time now. |
Our issue is that users need to select the specific bazel wrapper on the command line depending upon exactly where they are working, so overriding it in the refresh_compile_commands (hardcoded) is not quite enough. I could not figure out how to enable overriding such a parameter on the command line of the invocation of |
It's possible my need could be satisfied by yours by adding another layer of indirection - a new wrapper rule whose implementation pulls the var from ctx and then plugs that in as an argument to invocation of the refresh_compile_commands ? But again, I am learning bazel as I go here... |
This PR allows the user to configure the
bazel
binary name (or path). This is useful for where I work since we use a wrapper around the binary (with a different name).