feat(singlejar): Add Log4j plugins cache combiner #22581

stevebarrau · 2024-05-29T11:11:08Z

In singlejar, add support for combining Log4j2 plugins cache file.

Log4j2 plugins are Java annotations collected by a compiler plugin into .dat files for Log4j2 runtime to find them fast. With correct dependency on a java_plugin, Bazel already runs the java plugin compiler correctly. The behavior is correct on bazel run JAR, but not on _deploy.jar fat jars.

The silent clobbering of files in general, and the example of these Log4j2 .dat files in particular, is discussed in #7330.

File structure of Log4j2Plugins.dat files
the test data was generated by a python script, happy to commit it as well
How to merge Log4j2Plugins.dat?

I tested this fix in my project using Bazel 7.0.2, compiling //src:java_tools_prebuilt.zip, and overriding @remote_java_tools_darwin_arm64 and @remote_java_tools_linux.

stevebarrau · 2024-05-30T06:41:12Z

@sgowroji what question do I need to answer for "awaiting-user-response" to get removed?

@softprops @jwilliams-ocient from #7330; does this help the issue for you?

sgowroji · 2024-05-30T06:42:51Z

@stevebarrau Could you please take a look at the failing checks?

stevebarrau · 2024-05-30T10:34:21Z

@sgowroji PTAL.

hvadehra · 2024-05-31T08:17:16Z

cc @cushon

stevebarrau · 2024-07-22T21:47:07Z

Ping @cushon

meteorcloudy · 2024-08-20T17:48:56Z

@cushon Can you please take a look?

cushon · 2024-08-20T21:47:38Z

@hvadehra @meteorcloudy for Blaze we don't need to support log4j, so supporting this isn't a priority for me. I also don't want to stand in the way of progress if you want to support this in Bazel. I left a note in #7330 about the idea of trying to make this kind of thing more pluggable, but there may not be great alternatives to doing it directly in singlejar. If that's something you want to pursue, perhaps the new 'combiner' here could be factored into a separate file and only wired up for Bazel.

cushon · 2024-08-20T21:45:41Z

src/tools/singlejar/combiners.cc

-    return nullptr;
-  }
-
+void *outputEntryFromBuffer(const std::string filename,


Instead of extracting this helper, would it be possible to share implementation with Concatenator with composition, similar to what ManifestCombiner does?

Composed in e416b51

cushon · 2024-08-20T21:48:25Z

src/tools/singlejar/combiners.cc

@@ -284,3 +295,152 @@ void *ManifestCombiner::OutputEntry(bool compress) {
  concatenator_->Append("\r\n");
  return concatenator_->OutputEntry(compress);
 }
+
+template<typename T>


Would it be possible to move Log4J2PluginDatCombiner into a separate file?

Extracted in 8e901f0

stevebarrau · 2024-08-21T06:37:13Z

@cushon I would prefer to author this in Java to reuse the log4j merger logic from existing implementation. Ideally I agree with you if we could provide a set of custom combiners as configuration instead of having to add complexity/features in singlejar this would be very nice.

Where could we start to pry open singlejar and allow external combiners to be configured?

In the short term, if I address the comments, is this something that could live in singlejar until we rollout a configurability mechanism?

stevebarrau · 2024-08-21T06:39:18Z

@shs96c if this ends up being authorable in Java, would https://github.com/bazel-contrib/rules_jvm be a good home for this log4j jar combiner extension?

shs96c · 2024-08-21T07:47:31Z

contrib_rules_jvm is too high a level. You might want this in rules_jvm_external so maven artifacts we create are correct, but the only way for this to work with a deploy jar is to have this in rules_Java or wherever singlejar lives.

stevebarrau · 2024-10-01T15:43:28Z

@cushon PTAL.

stevebarrau · 2024-10-10T10:14:02Z

Friendly ping @cushon. Ideally I would like this to make it into Bazel 8 and a 7 point release.

stevebarrau · 2024-11-12T10:59:13Z

Friendly ping @cushon.

cushon · 2024-11-12T18:11:32Z

Sorry for the delay.

Thanks for refactoring the combiner into a separate file, I'm fine with the current approach. (Internally we use a separate singlejar entry point with additional combiners, we may not wire up the new combiner there, which is fine.)

@hvadehra do you want to review for Bazel?

hvadehra

My c++ is rusty but overall LGTM

the test data was generated by a python script, happy to commit it as well

Please do, and it would be great if we could generate the jar/dat outputs on the fly for the tests (perhaps in a genrule) rather than check them in.

stevebarrau · 2024-11-14T14:33:30Z

I am not sure I understand the ask here.

If the ask is for the 2 input jar to we generated by a genrule: these jar files are not stable because of zip data IIUC and they would be cached. Having them on disk circumvents this.

If the ask is Bazel does not want to suffer a zx kind of scenario, we can audit the python code and generate those jars in a trusted context (or directly inspect the JARs, they are a single file).

hvadehra

The PR LGTM as is, not having to check in semi-inscrutable inputs would have just been nicer.

I'd like to get @pzembrod's input on the c++ code before importing.

pzembrod · 2024-11-15T14:41:38Z

src/tools/singlejar/log4j2_plugin_dat_combiner.cc

+}
+
+uint32_t readInt(std::istringstream &stream) {
+  uint32_t values;


Curious nit: why plural "values"?

No particular reason, updated in c80889e.

pzembrod · 2024-11-15T15:07:03Z

src/tools/singlejar/log4j2_plugin_dat_combiner.cc

+std::string readUTFString(std::istringstream &stream) {
+  uint16_t length;
+  stream.read(reinterpret_cast<char *>(&length), sizeof(length));
+  length = swapByteOrder(length); // Convert to host byte order


Since I totally lack context here - and a future code maintainer may do so, too: Could you add a sentence or two more as to why the byte order needs to be swapped? And maybe the best place for that comment would be at the swapByteOrder() function itself?
I'm guessing it is because JVM class files are big-endian, and x86 and most ARMs are little-endian?
I guess there is not much big-endian hardware around these days, but I may be wrong, it's a long time since last I encountered endianness as a topic.

I added a comment to that effect in c80889e.

…uires compiler flag '/std:c++17'

stevebarrau · 2024-11-20T11:23:40Z

Ping @cushon; IIUC we got approval from the Bazel and C++ side for this change.

stevebarrau · 2024-12-02T13:43:03Z

Friendly ping @cushon

cushon · 2024-12-02T17:40:08Z

Friendly ping @cushon

I'm deferring to @hvadehra to review

hvadehra · 2024-12-03T09:28:18Z

It appears that importing this will need a bit of work, I'll do that myself.

fzakaria · 2024-12-09T22:31:01Z

Can this be merged into 7.5.0 ? (or whatever a 7.X release it can go into)

hvadehra · 2024-12-10T08:07:00Z

Can this be merged into 7.5.0 ? (or whatever a 7.X release it can go into)

That shouldn't be necessary. Once we have a java_tools / rules_java release with this change, one can just update those deps and stay on Bazel 7.

fzakaria · 2025-01-08T17:11:32Z

@hvadehra I'm looking at java_tools but it's not clear how to map the release back to a specific Bazel commit?
I"m trying to prove to myself what version of rules_java has this fix.

hvadehra · 2025-01-09T09:57:25Z

The java_tools releases have their provenance information embedded in the archives. For eg: if you look at https://github.com/bazelbuild/java_tools/releases/tag/java_v13.9 , download any one of the zip archives and look at the top-level README file inside.

The fix from this PR has not made it into a java_tools / rules_java release just yet. You can check the state at #24696 / bazelbuild/java_tools#93

fzakaria · 2025-01-09T16:58:58Z

Cool thanks;
I saw there are tags for the releases -- maybe I'l open a suggestion if it's possible to commit to the repo the provenance so that it's browsable in the GitHub repo.

stevebarrau changed the title ~~feat(singlejar): Add Log4j2Plugins combiner~~ feat(singlejar): Add Log4j plugins cache combiner May 29, 2024

github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels May 29, 2024

sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 29, 2024

stevebarrau force-pushed the singlejar-add-log4j-plugin-cache-combiner branch from 659623f to d8130cb Compare May 30, 2024 09:06

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels May 30, 2024

meteorcloudy requested a review from cushon August 20, 2024 17:48

cushon reviewed Aug 20, 2024

View reviewed changes

stevebarrau force-pushed the singlejar-add-log4j-plugin-cache-combiner branch 2 times, most recently from 3a95585 to 6ae3fb8 Compare October 1, 2024 13:19

stevebarrau requested a review from cushon October 1, 2024 15:43

cushon requested a review from hvadehra November 12, 2024 18:10

hvadehra reviewed Nov 13, 2024

View reviewed changes

hvadehra requested a review from pzembrod November 13, 2024 08:19

hvadehra approved these changes Nov 14, 2024

View reviewed changes

pzembrod approved these changes Nov 15, 2024

View reviewed changes

stevebarrau added 9 commits November 20, 2024 11:15

feat(singlejar): Add Log4j2Plugins combiner

ddb2c3b

fix: Load htonl and ntohl in Windows

acb0b00

fix: Do not use hton/ntoh functions

3c9b65f

fix: error C2429: language feature 'init-statements in if/switch' req…

1657bfb

…uires compiler flag '/std:c++17'

compose a Concatenator instead of extracting a helper

dcd2270

extract Log4J2PluginDatCombiner

5584081

fix //src/test/shell/bazel:bazel_java_tools_test

17f59d1

Add test data generation script

5b47417

Addresse review comments

c80889e

stevebarrau force-pushed the singlejar-add-log4j-plugin-cache-combiner branch from 6021715 to c80889e Compare November 20, 2024 11:21

cushon removed their request for review December 2, 2024 17:40

hvadehra 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 awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Dec 3, 2024

hvadehra self-assigned this Dec 3, 2024

copybara-service bot closed this in cd4580f Dec 9, 2024

srdo mentioned this pull request Dec 30, 2024

How do I make fat jars with scala_binary? bazelbuild/rules_scala#1673

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(singlejar): Add Log4j plugins cache combiner #22581

feat(singlejar): Add Log4j plugins cache combiner #22581

stevebarrau commented May 29, 2024 •

edited

Loading

stevebarrau commented May 30, 2024

sgowroji commented May 30, 2024

stevebarrau commented May 30, 2024

hvadehra commented May 31, 2024

stevebarrau commented Jul 22, 2024

meteorcloudy commented Aug 20, 2024

cushon commented Aug 20, 2024

cushon Aug 20, 2024

stevebarrau Oct 1, 2024

cushon Aug 20, 2024

stevebarrau Oct 1, 2024

stevebarrau commented Aug 21, 2024

stevebarrau commented Aug 21, 2024

shs96c commented Aug 21, 2024

stevebarrau commented Oct 1, 2024

stevebarrau commented Oct 10, 2024 •

edited

Loading

stevebarrau commented Nov 12, 2024

cushon commented Nov 12, 2024

hvadehra left a comment

stevebarrau commented Nov 14, 2024

hvadehra left a comment

pzembrod Nov 15, 2024

stevebarrau Nov 20, 2024

pzembrod Nov 15, 2024

stevebarrau Nov 20, 2024

stevebarrau commented Nov 20, 2024

stevebarrau commented Dec 2, 2024

cushon commented Dec 2, 2024

hvadehra commented Dec 3, 2024

fzakaria commented Dec 9, 2024

hvadehra commented Dec 10, 2024

fzakaria commented Jan 8, 2025

hvadehra commented Jan 9, 2025

fzakaria commented Jan 9, 2025

feat(singlejar): Add Log4j plugins cache combiner #22581

feat(singlejar): Add Log4j plugins cache combiner #22581

Conversation

stevebarrau commented May 29, 2024 • edited Loading

stevebarrau commented May 30, 2024

sgowroji commented May 30, 2024

stevebarrau commented May 30, 2024

hvadehra commented May 31, 2024

stevebarrau commented Jul 22, 2024

meteorcloudy commented Aug 20, 2024

cushon commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevebarrau commented Aug 21, 2024

stevebarrau commented Aug 21, 2024

shs96c commented Aug 21, 2024

stevebarrau commented Oct 1, 2024

stevebarrau commented Oct 10, 2024 • edited Loading

stevebarrau commented Nov 12, 2024

cushon commented Nov 12, 2024

hvadehra left a comment

Choose a reason for hiding this comment

stevebarrau commented Nov 14, 2024

hvadehra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevebarrau commented Nov 20, 2024

stevebarrau commented Dec 2, 2024

cushon commented Dec 2, 2024

hvadehra commented Dec 3, 2024

fzakaria commented Dec 9, 2024

hvadehra commented Dec 10, 2024

fzakaria commented Jan 8, 2025

hvadehra commented Jan 9, 2025

fzakaria commented Jan 9, 2025

stevebarrau commented May 29, 2024 •

edited

Loading

stevebarrau commented Oct 10, 2024 •

edited

Loading