Skip to content

COMPRESS-721 -- avoid reflection when possible in unpack200#764

Open
tballison wants to merge 7 commits intoapache:masterfrom
tballison:COMPRESS-721
Open

COMPRESS-721 -- avoid reflection when possible in unpack200#764
tballison wants to merge 7 commits intoapache:masterfrom
tballison:COMPRESS-721

Conversation

@tballison
Copy link

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • [*] Read the contribution guidelines for this project.
  • [ *] Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • [* ] I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
    Claude Opus 4.6 significantly
  • [* ] Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • [* ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [ *] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

@tballison
Copy link
Author

I added unit tests that fail when building without the ${argLine} --add-opens java.base/java.io=ALL-UNNAMED in the pom.xml

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hello @tballison

-1: These tests don't test anything new. They always pass, whether or not the changes to main are applied, inviting a regression to whatever you think you're fixing.

@tballison
Copy link
Author

tballison commented Mar 12, 2026

@garydgregory LOL...y, I added those when I was manually testing by removing the java >=17 profile in the pom.xml. However, as you point out, the build already fails spectacularly with existing tests when that is removed because of exactly this issue.

@tballison
Copy link
Author

As a side note, the more aggressive fix is to get rid of reflection entirely. I chose to modify the existing code as little as possible on this PR, but happy to go bigger if desired.

@tballison
Copy link
Author

Sorry for the noise, LegacyConstructorsTest obv still needs the java 17 profile. I confirmed we do not need new unit tests for pack200. The {{--add-opens}} which is necessary for existing unit tests masks the problems for unpack200.

@garydgregory
Copy link
Member

Hello @tballison

There might be a way to do this without reflection, if so please provide it 😁 Otherwise, these untested changes are Just asking for a regression in the future...

@tballison tballison requested a review from garydgregory March 12, 2026 13:12
@tballison
Copy link
Author

fix incoming, again apologies for the noise.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants