-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Bazel: add an install
shortcut and an experimental
attribute to codeql_pack
#18023
Conversation
This makes the first `codeql_pack` in a package add an `installer` target aliasing the `<name>-installer` one. This makes it so that one can for example do `bazel run //rust:installer` instead of the stuttering `bazel run //rust:rust-installer`. If a bazel package defines multiple `codeql_pack` targets, the first one only will get the `installer` alias.
installer
shortcut to codeql_pack
install
shortcut to codeql_pack
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.
Approving if you still want to merge this, despite the point I'm raising.
misc/bazel/pkg.bzl
Outdated
@@ -474,6 +475,8 @@ def codeql_pack( | |||
visibility = ["//visibility:public"], | |||
) | |||
_codeql_pack_install(internal("installer"), [name], install_dest = install_dest, apply_pack_prefix = False) | |||
if not native.existing_rule("install"): |
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.
I don't like this implementation - per https://bazel.build/rules/lib/toplevel/native#existing_rule, this API will be deprecated and removed at some point:
If possible, use this function only in implementation functions of rule finalizer symbolic macros. Use of this function in other contexts is not recommened, and will be disabled in a future Bazel release; it makes BUILD files brittle and order-dependent.
Given the fairly small impact of this, I'm fine merging this now, but we will have to remove this feature again once bazel follows through with the removal of this function.
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.
good point! I wasn't even aware of that (or of the new macro definition framework)
I agree it's better to avoid that, and have changed the code to do that, and added an experimental
flag to aid the definition of experimental packs. Alas, I still needed existing_rule
because the internal repo defines two codeql_pack
s in a bazel package (the java single and multi kotlin variants). So for backward compatibility I've left that. Once the internal repo is updated (probably we can put install_dest = None
on all codeql_packs
there), we can remove TODO
s here with a third PR.
install
shortcut to codeql_pack
install
shortcut and an experimental
attribute to codeql_pack
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.
Thanks, that looks much better!
This makes the first
codeql_pack
in a package add aninstall
target aliasing the<name>-installer
one. This makes it so that one can for example dobazel run //rust:install
instead of the stutteringbazel run //rust:rust-installer
. If a bazel package defines multiplecodeql_pack
targets, the first one only will get theinstall
alias.