Skip to content

Extract proguard logic to new rules#29641

Open
katre wants to merge 3 commits into
bazelbuild:masterfrom
katre:push-syywlltpvxtx
Open

Extract proguard logic to new rules#29641
katre wants to merge 3 commits into
bazelbuild:masterfrom
katre:push-syywlltpvxtx

Conversation

@katre
Copy link
Copy Markdown
Collaborator

@katre katre commented May 25, 2026

  • Timestamp clearing now uses a small hermetic python script instead of relying on non-hermetic unzip and zip.
  • Also split proguard and timestamp update actions for better caching.
  • Moved fastutil proguard spec into third_party where it is used.

RELNOTES: None.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented May 25, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions Bot added the awaiting-review PR is awaiting review from an assigned reviewer label May 25, 2026
@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 25, 2026

The only test failure is an exec format error only on windows/arm64 (windows/x64 passes):

# Execution platform: //:default_host_platform
/usr/bin/bash: line 6: /c/b/4qcz3zc6/execroot/_main/external/rules_java++toolchains+remotejdk25_win_arm64/bin/jar: cannot execute binary file: Exec format error

This is pretty surprising: is there a misconfiguration with the windows/arm64 jdk?

@katre katre force-pushed the push-syywlltpvxtx branch from 96a1324 to 6f2c183 Compare May 25, 2026 14:42
@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 25, 2026

Hmm, adding a find call shows it really is an arm64 binary:

/usr/bin/bash: line 7: /c/b/k3n7yowv/execroot/_main/external/rules_java++toolchains+remotejdk25_win_arm64/bin/jar: cannot execute binary file: Exec format error
/c/b/k3n7yowv/execroot/_main/external/rules_java++toolchains+remotejdk25_win_arm64/bin/jar: PE32+ executable for MS Windows 6.02 (console), ARM64, 6 sections

@meteorcloudy , any ideas what's wrong here?

@katre katre requested a review from meteorcloudy May 25, 2026 14:46
@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 25, 2026

I'm also tempted to rewrite this as a python script, which would probably also fix the windows issue. I'll think about it and get back to this later.

@katre katre force-pushed the push-syywlltpvxtx branch from 6f2c183 to 0b5d43c Compare May 25, 2026 18:09
@katre katre changed the title Update fastutil_tripped to use hermetic jar instead of nonhermetic unzip Extract proguard logic to new rules May 25, 2026
@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 25, 2026

Went ahead and used the script, and added actual rules, and re-arranged things somewhat. PTAL.

@katre katre force-pushed the push-syywlltpvxtx branch from 0b5d43c to b0a8019 Compare May 27, 2026 00:43
- Timestamp clearing now uses a small hermetic python script instead of
  relying on non-hermetic `unzip` and `zip`.
- Also split proguard and timestamp update actions for better caching.
- Moved fastutil proguard spec into third_party where it is used.

RELNOTES: None.
@katre katre force-pushed the push-syywlltpvxtx branch from b0a8019 to cbcc559 Compare May 27, 2026 00:52
@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 27, 2026

All tests fixed, hooray.

@katre katre requested a review from hvadehra May 27, 2026 14:49
Comment thread tools/build_defs/proguard/proguard.bzl Outdated
executable = ctx.executable._proguard_bin,
arguments = [
"-injars",
",".join([src.path for src in ctx.files.srcs]),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Even though this is just a rule for Bazel's internal use, it would be great if we could follow best practices and use Args instead. Folks (and AI) like to copy 😉

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fair, I tend to write rules by copying code from https://github.com/bazelbuild/examples.

I'll look at adding an example of ctx.actions.args().

Comment thread tools/build_defs/proguard/proguard.bzl Outdated
)

def _fix_timestamps(ctx, input, output):
ctx.actions.run(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Running this as a separate action means that the previous one will have non-deterministic outputs. Perhaps the two should be merged?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the original genrule, they were merged, and I split them up mostly to make it easier to test the timestamp-clearing script (so I didn't have to keep re-running the slower proguard step).

I'll take a look at updating the script to call proguard and combine the two actions into one.

@katre katre requested a review from fmeum May 29, 2026 16:30
@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 29, 2026

Combined both steps into a single python wrapper.

@katre
Copy link
Copy Markdown
Collaborator Author

katre commented May 29, 2026

I don't know enough about python runfiles on windows to fix this: is it worth looking into, or should I revert it to use separate actions?


from python.runfiles import runfiles

PROGUARD_BIN_PATH = "_main/tools/build_defs/proguard/proguard_bin"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The path will likely end with .exe on Windows. In any case it's better to pass this in from location expansion of $(rlocationpath ...)

@iancha1992 iancha1992 added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-review PR is awaiting review from an assigned reviewer team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants