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

Bugfix/java checks #1228

Merged
merged 3 commits into from May 9, 2021
Merged

Bugfix/java checks #1228

merged 3 commits into from May 9, 2021

Conversation

breautek
Copy link
Contributor

@breautek breautek commented May 9, 2021

Platforms affected

Android

Motivation and Context

Fixes #1221
Fixes #1203 (Addressed by #1220)

Description

This addresses two flaws in Java checking.

  1. If some Java options are present, javac -version will print out additional lines of information. A regex is used to find the java(c) x.x.x line for proper version checking. Addressed by fix(requirements check): use regex to get java version from javac output #1220
  2. If JAVA_HOME is set, we would only set the path if javac wasn't findable, and when we do, we set the path at the end of the PATH chain. To be smarter, if JAVA_HOME is set, we always want to set the JAVA_HOME bin path to the front of the PATH chain so that we can guarantee the proper java executable to be used.

Testing

Added new test for the javac -version issue. All existing tests passes. Manual testing for the JAVA_HOME issue.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@breautek breautek added the bug label May 9, 2021
@breautek breautek added this to the 10.0.0 milestone May 9, 2021
@breautek breautek marked this pull request as ready for review May 9, 2021 00:47
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2021

Codecov Report

Merging #1228 (98cd058) into master (ae4dba2) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
- Coverage   72.02%   72.00%   -0.02%     
==========================================
  Files          21       21              
  Lines        1698     1697       -1     
==========================================
- Hits         1223     1222       -1     
  Misses        475      475              
Impacted Files Coverage Δ
bin/templates/cordova/lib/env/java.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae4dba2...98cd058. Read the comment docs.

@breautek breautek added this to In Progress in Release Plan - 10.1.0 via automation May 9, 2021
Release Plan - 10.1.0 automation moved this from In Progress to Reviewer approved May 9, 2021
Copy link
Contributor

@PieterVanPoyer PieterVanPoyer left a comment

Choose a reason for hiding this comment

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

Great!

@breautek breautek mentioned this pull request May 9, 2021
4 tasks
@breautek breautek merged commit 6d803e2 into apache:master May 9, 2021
Release Plan - 10.1.0 automation moved this from Reviewer approved to Done May 9, 2021
@breautek breautek deleted the bugfix/java-checks branch May 9, 2021 20:52
wedgberto pushed a commit to wedgberto/cordova-android that referenced this pull request May 17, 2022
* fix: Java version parsing if java executable prints out additional information with --version

* fix: Ensure JAVA_HOME path comes first in the PATH environment

* refactor: Removed redundent code in favour of keeping a change introduced from another PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
3 participants