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

Strip unnecessary Bazel server jar contents #22787

Closed
wants to merge 3 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jun 18, 2024

fastutil is very large and thus stripped (but not obfuscated) with proguard. The Auto* processors do not need to be included into the release binary, only their annotations.

Also removes some third_party targets that are unused.

Bazel binary size on macOS is 65 MB before and 53 MB after this change.

Work towards #22722

@fmeum fmeum changed the title Remove unnecessary Bazel server jar contents Strip unnecessary Bazel server jar contents Jun 18, 2024
@fmeum fmeum requested a review from meteorcloudy June 18, 2024 12:34
@fmeum fmeum marked this pull request as ready for review June 18, 2024 12:34
@fmeum fmeum requested a review from meisterT June 18, 2024 12:34
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jun 18, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 18, 2024

@meteorcloudy This PR touches third_party in a non-trivial way: It adds Maven dependencies that //third_party will depend on and also has to adjust a test that depends on //third_party. I don't know how I need to split this up.

@meteorcloudy
Copy link
Member

No worries, the third_party files touched in this PR are all imported, so we can just go through the normal import process.

@meteorcloudy
Copy link
Member

meteorcloudy commented Jun 18, 2024

The complete list of imported files are:

    return glob(
        include = ["**"],
        # Keep existing sources from git.
        exclude = [
            "CHANGELOG.md",
            "COPYRIGHT",
            "LICENSE.txt",
            "third_party/android_dex/**",
            "third_party/asm/**",
            "third_party/googleapis/**",
            "third_party/def_parser/**",
            "third_party/grpc-java/**",
            "third_party/grpc/**",
            "third_party/jarjar/**",
            "third_party/java/**",
            "third_party/pprof/**",
            "third_party/py/**",
            "third_party/remoteapis/**",
            "third_party/turbine/**",
            "third_party/zlib/**",
        ],
    ) + glob(include = ["third_party/**/MODULE.bazel"])

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 18, 2024

@meisterT This required some after the fact fixes to the JAR to make it deterministic.

@fmeum fmeum requested a review from meisterT June 18, 2024 14:07
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jun 18, 2024
@sgowroji sgowroji added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 18, 2024
@@ -0,0 +1,8 @@
-dontobfuscate
Copy link
Member

Choose a reason for hiding this comment

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

@fmeum Unfortunately, our internal policy doesn't allow importing this file under bazel's own third_party to prevent potential bundling dependencies. I assume this is authored by you, right? Can you move this to the tools directory instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean that I should move the file into tools, but keep the genrule referencing it in third_party? Wouldn't that cause the file to show up under @bazel_tools//tools?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, as long as it's not included in

name = "embedded_tools_srcs",
, it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

So maybe under tools/proguard?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it into tools and added it to exclude so that excluding it is documented as an explicit choice.

Copy link
Member

Choose a reason for hiding this comment

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

SG, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

To fix the CI breakage, you probably have to sync to HEAD.

fastutil is very large and thus stripped (but not obfuscated) with proguard.

Also removes some third_party maven deps that are unused.
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jun 24, 2024
@fmeum fmeum deleted the 22722-smaller-binary branch June 25, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants