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

GH-481: Fix bug where large builds crash on Windows #482

Merged
merged 4 commits into from
Dec 7, 2024

Conversation

ascopes
Copy link
Owner

@ascopes ascopes commented Dec 7, 2024

This change implements the argument file mechanism for protoc
command line flags, like we do already for Java invocations. This
helps avoid build failures on Windows when a large number of files
are being generated, since Windows has somewhat esoteric limits
on the command line length. Offloading this to a file works around
this constraint.

A reproduction of the issue for Windows is also included in the test
pack, which generates a large number of random protobuf sources on
each build and feeds them into the Maven plugin.

A fix for Mockito has also been included in this change to squelch
warnings about agent attachments that I was encountering on Java 21
builds.

Fixes GH-481.

This change implements the argument file mechanism for protoc
command line flags, like we do already for Java invocations. This
helps avoid build failures on Windows when a large number of files
are being generated, since Windows has somewhat esoteric limits
on the command line length. Offloading this to a file works around
this constraint.
@ascopes ascopes added the bug Something isn't working label Dec 7, 2024
@ascopes ascopes added this to the v2.7.x milestone Dec 7, 2024
@ascopes ascopes self-assigned this Dec 7, 2024
@ascopes ascopes enabled auto-merge (squash) December 7, 2024 14:27
@ascopes ascopes disabled auto-merge December 7, 2024 14:27
@ascopes ascopes enabled auto-merge (squash) December 7, 2024 14:27
Copy link

codecov bot commented Dec 7, 2024

Codecov Report

Attention: Patch coverage is 92.96875% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.54%. Comparing base (3c510be) to head (88572e9).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...protobufmavenplugin/plugins/JvmPluginResolver.java 84.45% 7 Missing ⚠️
...enplugin/generation/ProtobufBuildOrchestrator.java 94.45% 0 Missing and 1 partial ⚠️
...lugin/protoc/ProtocArgumentFileBuilderBuilder.java 96.56% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
+ Coverage   87.31%   87.54%   +0.24%     
==========================================
  Files          37       38       +1     
  Lines        1252     1276      +24     
  Branches       92       91       -1     
==========================================
+ Hits         1093     1117      +24     
  Misses        119      119              
  Partials       40       40              
Files with missing lines Coverage Δ
...rotobufmavenplugin/protoc/CommandLineExecutor.java 78.73% <100.00%> (+3.12%) ⬆️
...protobufmavenplugin/utils/ArgumentFileBuilder.java 100.00% <100.00%> (ø)
...b/ascopes/protobufmavenplugin/utils/TeeWriter.java 100.00% <100.00%> (ø)
...enplugin/generation/ProtobufBuildOrchestrator.java 83.00% <94.45%> (+0.18%) ⬆️
...lugin/protoc/ProtocArgumentFileBuilderBuilder.java 92.60% <96.56%> (ø)
...protobufmavenplugin/plugins/JvmPluginResolver.java 82.90% <84.45%> (+0.35%) ⬆️

@ascopes ascopes disabled auto-merge December 7, 2024 14:38
@ascopes ascopes enabled auto-merge (squash) December 7, 2024 14:38
@ascopes ascopes force-pushed the bugfix/GH-481-command-length branch from 1e69fba to 88572e9 Compare December 7, 2024 14:45
@ascopes ascopes merged commit 9949d73 into main Dec 7, 2024
10 checks passed
@ascopes ascopes deleted the bugfix/GH-481-command-length branch December 7, 2024 14:47
@ascopes
Copy link
Owner Author

ascopes commented Dec 7, 2024

...and with that, we learn that GitHub's auto merge feature is buggy and will merge without waiting for builds to succeed.

Sigh. Any further fixes will have to be done on a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: [Windows] The filename or extension is too long
1 participant