-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(requirements check): use regex to get java version from javac output #1220
Conversation
Yah I don't think we can remove the environment variables, as that may contain details like |
Will merge by tomorrow night if there are no objections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I did not test it.
Thanks for the review @PieterVanPoyer and thank you for your contribution @DavidStrausz |
This basically fixes up the changes from #1220. * test(env/java): replace test that duplicates implementation * test(env/java): stub _ensure to focus on unit under test * test(env/java): add test for invalid output * refactor(env/java): keep try block small * refactor(env/java): shorten excessive comment
…put (apache#1220) * fix(requirements check): use regex to get java version from javac output * fix(lint): format code * fix(node 10): remove optional chaining from version check
This basically fixes up the changes from apache#1220. * test(env/java): replace test that duplicates implementation * test(env/java): stub _ensure to focus on unit under test * test(env/java): add test for invalid output * refactor(env/java): keep try block small * refactor(env/java): shorten excessive comment
Platforms affected
Android
Motivation and Context
Fixes getting the java version from the
javac -version
output on macOS devices when custom_JAVA_OPTIONS
are set. A detailed description of the issue can be found im my comment here.Fixes #1203
Description
Use a regex to get just the version string from the
javac
output. Fixes #1130.Previously
semver.coerce()
would get a wrong version from a string like the following:With the change in this PR, the following regex is used to get the version string (
1.8.0
) from the output:/javac\s+([\d.]+)/i
I also tried removing
_JAVA_OPTIONS
from the version check by addingenv: {}, extendEnv: false
to the options of thejavac
command like suggested here:This changed the output from:
to:
which subsequently let the requirements check fail again (no idea where
12.0.2
comes from). So I just went with the regex solution.Testing
cordova build android
, check if requirements check succeedscordova build android
, check if requirements check succeedsjavac -version
output with_JAVA_OPTIONS
, run unit testsChecklist
(platform)
if this change only applies to one platform (e.g.(android)
)