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

pkg.bzl: Significantly restructure codeql_pack rule. #16723

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

criemen
Copy link
Collaborator

@criemen criemen commented Jun 11, 2024

This PR introduces a codeql_pack_rule that does the heavy lifting of extracting arch- and common zip files for production dist building. It also factors out the installer targets for individual packs, as well as pack groups.

This changes the contract between the internal build system and the pack definition significantly, which is why an accompanying internal PR is required. No backwards compatibility layer is provided, as the PR as complex enough as-is.

The individual codeql_pack rules are now much simpler, as they mostly stuff their inputs into a new _CodeQLPackInfo provider, and let the installer and codeql_pack_group rules do the heavy lifting. For working in the external repo with self-contained packs, the per-pack installer targets are still available. Internally, we'll only use the new codeql_pack_group targets going forward, both for defining intree-dists and for building the production zip files.

This PR introduces a `codeql_pack_rule` that does the heavy lifting
of extracting arch- and common zip files for production dist building.
It also factors out the installer targets for individual packs,
as well as pack groups.

This changes the contract between the internal build system and the pack
definition significantly, which is why an accompanying internal PR is required.
No backwards compatibility layer is provided, as the PR as complex enough as-is.

The individual `codeql_pack` rules are now much simpler,
as they mostly stuff their inputs into a new `_CodeQLPackInfo` provider,
and let the installer and `codeql_pack_group` rules do the heavy lifting.
For working in the external repo with self-contained packs,
the per-pack installer targets are still available.
Internally, we'll only use the new `codeql_pack_group` targets
going forward, both for defining intree-dists and for building
the production zip files.
@criemen criemen added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jun 11, 2024
@criemen criemen marked this pull request as ready for review June 11, 2024 13:20
@criemen criemen requested review from a team as code owners June 11, 2024 13:20
Turns out we need this for our production targets.
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

That's a lot of work, thanks for this! The swift-files thing is related to a comment of mine in the internal PR, but overall this looks really good!

zips_to_prefixes[zip] = prefix
return [
DefaultInfo(files = depset(zips_to_prefixes.keys(), transitive = [ctx.attr.src[DefaultInfo].files])),
_CodeQLPackInfo(arch_overrides = ctx.attr.arch_overrides, files = ctx.attr.src[PackageFilegroupInfo], zips = _ZipInfo(zips_to_prefixes = zips_to_prefixes), pack_prefix = ctx.attr.prefix),
Copy link
Contributor

Choose a reason for hiding this comment

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

cosmetic nit: maybe split this into multiple lines, seems like a pretty long one

misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

LGTM!

@criemen criemen merged commit c808953 into main Jun 14, 2024
19 of 22 checks passed
@criemen criemen deleted the criemen/codeql-pack-group branch June 14, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on internal PR This PR should only be merged in sync with an internal Semmle PR JS Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants