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 codeql specific packaging library #16432

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented May 6, 2024

This encapsulate arch specific logic, local installation and separation of zip files into generic and arch-specific parts as required by the internal build. The logic behind separating generic and arch-specific files lies in using {CODEQL_PLATFORM} in the prefix, which then gets expanded to the appropriate value.

Moreover, this PR the ability to import .zip files into the pack in a highly optimized way:

  • when installing the pack, ripunzip will be used to unzip the zip file contents to the destination prefix
  • when creating a zip file of the pack, a special purpose zipmerge (ported from the internal repository) merges the imported zips into the output, without decompressing.

These changes are applied to swift as an example.

@github-actions github-actions bot added the Swift label May 6, 2024
@redsun82 redsun82 mentioned this pull request May 6, 2024
@redsun82 redsun82 marked this pull request as ready for review May 21, 2024 08:06
@redsun82 redsun82 requested review from a team as code owners May 21, 2024 08:06
@redsun82 redsun82 requested a review from criemen May 21, 2024 08:08
@redsun82 redsun82 marked this pull request as draft May 23, 2024 12:05
This encapsulate arch specific logic, local installation and separation
of zip files into generic and arch-specific parts as required by the
internal build.
@redsun82 redsun82 marked this pull request as ready for review May 24, 2024 10:40
@redsun82 redsun82 marked this pull request as draft May 27, 2024 06:52
@redsun82 redsun82 marked this pull request as ready for review May 27, 2024 08:47
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! some initial comments, a lot of them around missing documentation. I'll take a second look at the rule implementation once the documentation is there, so I have a better idea how things are supposed to hang together.

misc/bazel/internal/install.py Outdated Show resolved Hide resolved
misc/bazel/internal/install.py Show resolved Hide resolved
misc/bazel/internal/zipmerge/BUILD.bazel Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Show resolved Hide resolved
misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
swift/BUILD.bazel Show resolved Hide resolved
swift/third_party/resource-dir/BUILD.bazel Show resolved Hide resolved
.bazelrc Show resolved Hide resolved
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.

One more round of review comments, thanks a lot for all the code comments, they've been really useful in following along.

misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
the requested `kind`.
""",
attrs = {
"base": attr.label(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a need to expose this separately from zips?
Couldn't we just merge the zip files in zips? I don't see the benefit of a base argument, but happy to be convinced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, after trying it again I remembered why I had done this. The distinction whether to select a zip or not depends on its prefix containing {CODEQL_PLATFORM}. The base zip would be added with an empty "" prefix, which would disqualify it from the arch zipmerge. So the base_zip is always included, while zips are included conditionally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, then a comment to that effect would be great, to explain that design choice to future us.

misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Show resolved Hide resolved
misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Outdated Show resolved Hide resolved
misc/bazel/pkg.bzl Show resolved Hide resolved
misc/bazel/internal/zipmerge/BUILD.bazel Show resolved Hide resolved
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 a lot, I think we're nearly there ❤️

srcs = [internal(kind)],
visibility = ["//visibility:private"],
package_dir = name,
package_file_name = name + "-" + (_detect_platform() if kind == "arch" else "generic") + ".zip",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we pull out this logic into a variable in codeql_pack, and remove the same/similar filename scheme from _zipmerge? That'd make zipmerge a little bit more generic, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

such an innocent looking comment 😝

I've ended up extracting the whole filtering + plat expansion logic to separate rules with a specific provider. This effectively seems like being on the good path to generalize _zipmerge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants