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

Bazel: add an install shortcut and an experimental attribute to codeql_pack #18023

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Nov 19, 2024

This makes the first codeql_pack in a package add an install target aliasing the <name>-installer one. This makes it so that one can for example do bazel run //rust:install 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 install alias.

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.
@redsun82 redsun82 requested a review from a team as a code owner November 19, 2024 09:28
@github-actions github-actions bot added Rust Pull requests that update Rust code Actions Analysis of GitHub Actions labels Nov 19, 2024
@redsun82 redsun82 requested a review from criemen November 19, 2024 09:29
@redsun82 redsun82 requested a review from a team as a code owner November 19, 2024 09:34
@redsun82 redsun82 changed the title Bazel: add an installer shortcut to codeql_pack Bazel: add an install shortcut to codeql_pack Nov 19, 2024
criemen
criemen previously approved these changes Nov 19, 2024
Copy link
Collaborator

@criemen criemen left a 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.

@@ -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"):
Copy link
Collaborator

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.

Copy link
Contributor Author

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_packs 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 TODOs here with a third PR.

@redsun82 redsun82 changed the title Bazel: add an install shortcut to codeql_pack Bazel: add an install shortcut and an experimental attribute to codeql_pack Nov 19, 2024
@redsun82 redsun82 requested a review from criemen November 19, 2024 11:54
Copy link
Collaborator

@criemen criemen left a 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!

@redsun82 redsun82 merged commit 2f3624b into main Nov 19, 2024
27 checks passed
@redsun82 redsun82 deleted the redsun82/installer-shortcut branch November 19, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Actions Analysis of GitHub Actions documentation Rust Pull requests that update Rust code Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants