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

detekt 1.22.0: Add --add-opens java.base/java.lang=ALL-UNNAMED for openjdk@17 #116785

Closed
wants to merge 3 commits into from

Conversation

rowi1de
Copy link
Contributor

@rowi1de rowi1de commented Nov 27, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@BrewTestBot BrewTestBot added the java Java use is a significant feature of the PR or issue label Nov 27, 2022
- With the upgrade from openjdk@11 to openjdk@17 the formula breaks when running detekt
- See Homebrew#116784
@rowi1de rowi1de changed the title [FIX] Detekt Add --add-opens java.base/java.lang=ALL-UNNAMED detekt 1.22.0: Add --add-opens java.base/java.lang=ALL-UNNAMED for openjdk@17 Nov 27, 2022
@SMillerDev
Copy link
Member

Is there no upstream patch for this yet?

@rowi1de
Copy link
Contributor Author

rowi1de commented Nov 27, 2022

Is there no upstream patch for this yet?

Unfortunately not and this error was introduced to the Formular in https://github.com/Homebrew/homebrew-core/blame/04c92a907a3c0a1cefa2e22aeee8a39cae9d47f6/Formula/detekt.rb#L17 when upgrading the JDK

@SMillerDev
Copy link
Member

Is there an upstream issue we can reference to know when we can remove this workaround?

@rowi1de
Copy link
Contributor Author

rowi1de commented Nov 27, 2022

Is there an upstream issue we can reference to know when we can remove this workaround?

It was discovered here detekt/detekt#5247 (comment)
as detekt wraps ktlint pinterest/ktlint#1195 which will send you here: https://youtrack.jetbrains.com/issue/KT-43704

most similar upstream issue pinterest/ktlint#1618

however this fix reduces the chance of the Formular / tool breaking in the future to to similar upstream changes

@SMillerDev
Copy link
Member

This does highlight an issue in detekt itself though, which I'll look into further

This comment there has me a bit worried. It might hide the symptoms if we merge this PR. But does it really fix anything?

@rowi1de
Copy link
Contributor Author

rowi1de commented Nov 27, 2022

This does highlight an issue in detekt itself though, which I'll look into further

This comment there has me a bit worried. It might hide the symptoms if we merge this PR. But does it really fix anything?

Yes 👍🏼 it makes the installed tool usable again.
Try the following:
#116784 (comment)
by just installing detekt with the unfixed Formular and this fixed one.

@3flex
Copy link

3flex commented Nov 28, 2022

This does highlight an issue in detekt itself though, which I'll look into further

That comment was from me. There's nothing we can change in detekt itself to fix this. All I meant by that comment was that the generated scripts used by detekt itself don't currently include that flag, but should, so it's fixed for other users who are using the CLI distribution of detekt.

I'd suggest actually changing the homebrew script to download the ZIP (e.g. detekt-cli-1.22.0.zip), then call /path/to/unzipped/location/bin/detekt-cli. That script should include the workaround in a future release. But for 1.22.0 you will want the workaround proposed here.

@3flex
Copy link

3flex commented Nov 28, 2022

Actually it might be possible to fix this for the JAR as well by adding an appropriate Add-Opens attribute to MANIFEST.MF. I will need to test.

Regardless of how we manage this in detekt itself though, this workaround is required in the formula file to make v1.22.0 work in brew. The workaround might not be required for future versions, you can track this for updates: detekt/detekt#5576

Formula/detekt.rb Outdated Show resolved Hide resolved
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
java Java use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
4 participants