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

use sed entirely with regex to get the java major version #1544

Merged
merged 4 commits into from Jul 21, 2022

Conversation

rawilder
Copy link
Contributor

@rawilder rawilder commented Jul 13, 2022

Description

Fix java version being incorrectly set when JAVA_TOOL_OPTIONS has a value. It being set causes unexpected output for the current java version parsing method.

Investigation

This seems to be happening from these two lines in the ktlint start

# incorrectly retrieves version
JV=$(java -version 2>&1 | head -1 | cut -d'"' -f2 | sed '/^1\./s///' | cut -d'.' -f1)
echo $JV
# echoes 'Picked up JAVA_TOOL_OPTIONS:'
X=$( [ "$JV" -ge "16" ] && echo "--add-opens java.base/java.lang=ALL-UNNAMED" || echo "")
# this command fails because the version is not correctly parsed

Ktlint still runs okay, it just spits some errors in the beginning. This could be solved with a different way to get the java version. Perhaps something with sed or awk to search for the 'version' string instead of head.

Local test results

A couple of tests failed locally but the same tests failed on master for me locally.

CLI basic checks > Given CLI argument --help then return the help output() FAILED
    org.opentest4j.AssertionFailedError at SimpleCLITest.kt:14

CLI basic checks > Given CLI argument --version then return the version information output() FAILED
    org.opentest4j.AssertionFailedError at SimpleCLITest.kt:29

CLI basic checks > Given some code without errors then return from lint with normal exit code and no error output() FAILED
    org.opentest4j.AssertionFailedError at SimpleCLITest.kt:45

Closes #1543

Checklist

  • PR description added
  • tests are added // is this something I can test considering it is in the build execution itself?
  • CHANGELOG.md is updated

CHANGELOG.md Outdated Show resolved Hide resolved
ktlint/build.gradle.kts Show resolved Hide resolved
@paul-dingemans
Copy link
Collaborator

paul-dingemans commented Jul 14, 2022

I am just reading the issue #1543 after making my review remarks. That issue clarifies already a lot.

@rawilder
Copy link
Contributor Author

Would you like me to update the PR description with text from the issue to make it clearer what this is doing?

@paul-dingemans
Copy link
Collaborator

Would you like me to update the PR description with text from the issue to make it clearer what this is doing?

That would have helped with the initial review. It would be great if you can still update it though for future reference.

@rawilder
Copy link
Contributor Author

Updated description with more info.

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.

ktlint prints an error when running with JAVA_TOOL_OPTIONS
2 participants