Skip to content

Conversation

@andrewkolos
Copy link
Contributor

Summary

Fixes #139180, where flutter create could crash if the java binary the tool found cannot be run.

Context

At startup, the tool searches for a Java installation1. Unless the located installation is from an Android Studio installation, the tool does not verify that the binary is runnable. For more, see #139613, which tracks this inconsistency in behavior.

This means that in the scenario where

  1. the user does not have Android Studio installed or the java binary found within cannot be run and
  2. the user has a) flutter config --jdk-dir set, b) JAVA_HOME set in their environment, or c) java on their system path and
  3. the java binary we think we found during cannot be run (or java --version fails), then

the user running flutter create with Android enabled will hit a tool crash.

Change

Java.version should return null if version checking fails for any reason. This is documented behavior. Therefore, we'll update the implementation to first verify that the binary is runnable. If it isn't, it will return null.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on [Discord].

Footnotes

  1. We find java by calling the static Java.find, see: https://github.com/flutter/flutter/blob/48187028c11ca8ca10e0179705d25553e1fe2c14/packages/flutter_tools/lib/src/context_runner.dart#L271

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 6, 2023
@andrewkolos andrewkolos changed the title have Java.version return null if java binary cannot be run have Java.version return null if java --version cannot be run Dec 6, 2023
@andrewkolos andrewkolos changed the title have Java.version return null if java --version cannot be run have Java.version return null if java --version fails or cannot be run Dec 6, 2023
@christopherfujino
Copy link
Contributor

christopherfujino commented Dec 6, 2023

So after this change what will happen during the flutter create workflow? And would it not be better to check this in Java.find() and throwToolExit there if we resolved to a path that COULDN'T be run?

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Dec 6, 2023

So after this change what will happen during the flutter create workflow?
Oops, I forgot to discuss this. See #139180 (comment) for context. Basically, this change would make the tool not check compatibility during flutter create if Java is unavailable. However, I think you're onto something here. Maybe we should print a warning when flutter create includes Android but Java cannot be run. We could also throwToolExit if we want to be more opinionated about it.

One question I have is: is flutter create the only time we check for Java compatibility? If it is, we may want to make this a ToolExit. However, if it's not, to keep this change as easy to cherry-pick as possible, a warning about Java being unavailable could suffice. I will follow up in the android channel in discord.

And would it not be better to check this in Java.find() and throwToolExit there if we resolved to a path that COULDN'T be run?

Maybe we should, but such a drastic change could be dangerous to cherry-pick. I've filed #139613 to track this.

@christopherfujino
Copy link
Contributor

So after this change what will happen during the flutter create workflow?
Oops, I forgot to discuss this. See #139180 (comment) for context. Basically, this change would make the tool not check compatibility during flutter create if Java is unavailable. However, I think you're onto something here. Maybe we should print a warning when flutter create includes Android but Java cannot be run. We could also throwToolExit if we want to be more opinionated about it.

One question I have is: is flutter create the only time we check for Java compatibility? If it is, we may want to make this a ToolExit. However, if it's not, to keep this change as easy to cherry-pick as possible, a warning about Java being unavailable could suffice. I will follow up in the android channel in discord.

And would it not be better to check this in Java.find() and throwToolExit there if we resolved to a path that COULDN'T be run?

Maybe we should, but such a drastic change could be dangerous to cherry-pick. I've filed #139613 to track this.

Ahh yes, this is a good point. I will approve this as is so we can cherry-pick it to the release and avoid crashes. Any other types of changes we probably wouldn't want to cherry-pick, and thus should happen in a later change.

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2023
@auto-submit auto-submit bot merged commit c22c19b into flutter:master Dec 6, 2023
andrewkolos added a commit to andrewkolos/flutter that referenced this pull request Dec 6, 2023
…run (flutter#139614)

## Summary
Fixes flutter#139180, where `flutter create` could crash if the `java` binary the tool found cannot be run.

## Context

At startup, the tool searches for a Java installation[^1]. Unless the located installation is from [an Android Studio installation](https://github.com/flutter/flutter/blob/e1967ecabf014bf93d1731fde6a6547b06ca9c33/packages/flutter_tools/lib/src/android/android_studio.dart#L163), the tool does not verify that the binary is runnable. For more, see flutter#139613, which tracks this inconsistency in behavior. 

This means that in the scenario where

1) the user does not have Android Studio installed or the java binary found within cannot be run **and**
2) the user has a) `flutter config --jdk-dir` set, b) `JAVA_HOME` set in their environment, **or** c) `java` on their system path **and**
3) the java binary we think we found during cannot be run (or `java --version` fails), **then**

the user running `flutter create` with Android enabled will hit a tool crash.

## Change

`Java.version` should return null if version checking fails for any reason. [This is documented behavior](https://github.com/flutter/flutter/blob/48f57621ade657b0c20ba53d513de4c3cd563abd/packages/flutter_tools/lib/src/android/java.dart#L136). Therefore, we'll update the implementation to first verify that the binary is runnable. If it isn't, it will return `null`.

[^1]: We find java by calling the static `Java.find`, see: https://github.com/flutter/flutter/blob/48187028c11ca8ca10e0179705d25553e1fe2c14/packages/flutter_tools/lib/src/context_runner.dart#L271
[^2]: This PR doesn't change this, as this would be too dangerous to cherry-pick into stable.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 8, 2023
andrewkolos added a commit to andrewkolos/flutter that referenced this pull request Dec 8, 2023
…run (flutter#139614)

## Summary
Fixes flutter#139180, where `flutter create` could crash if the `java` binary the tool found cannot be run.

## Context

At startup, the tool searches for a Java installation[^1]. Unless the located installation is from [an Android Studio installation](https://github.com/flutter/flutter/blob/e1967ecabf014bf93d1731fde6a6547b06ca9c33/packages/flutter_tools/lib/src/android/android_studio.dart#L163), the tool does not verify that the binary is runnable. For more, see flutter#139613, which tracks this inconsistency in behavior. 

This means that in the scenario where

1) the user does not have Android Studio installed or the java binary found within cannot be run **and**
2) the user has a) `flutter config --jdk-dir` set, b) `JAVA_HOME` set in their environment, **or** c) `java` on their system path **and**
3) the java binary we think we found during cannot be run (or `java --version` fails), **then**

the user running `flutter create` with Android enabled will hit a tool crash.

## Change

`Java.version` should return null if version checking fails for any reason. [This is documented behavior](https://github.com/flutter/flutter/blob/48f57621ade657b0c20ba53d513de4c3cd563abd/packages/flutter_tools/lib/src/android/java.dart#L136). Therefore, we'll update the implementation to first verify that the binary is runnable. If it isn't, it will return `null`.

[^1]: We find java by calling the static `Java.find`, see: https://github.com/flutter/flutter/blob/48187028c11ca8ca10e0179705d25553e1fe2c14/packages/flutter_tools/lib/src/context_runner.dart#L271
[^2]: This PR doesn't change this, as this would be too dangerous to cherry-pick into stable.
auto-submit bot pushed a commit that referenced this pull request Dec 11, 2023
…not be run (#139698)

cherry-picks changes from #139614 onto the stable channel
auto-submit bot pushed a commit that referenced this pull request Dec 12, 2023
…r cannot be run (#139838)

cherry-picks changes from #139614 onto the beta channel
bryanoltman added a commit to shorebirdtech/flutter that referenced this pull request Dec 20, 2023
* [CP] Gold fix for stable branch (flutter#139764)

Fixes flutter#139673
Cherry picks flutter#139706 to the stable branch to fix the tree.

* [macOS] Suppress Xcode 15 createItemModels warning (flutter#138243) (flutter#139782)

As of Xcode 15 on macOS Sonoma, the following message is (repeatedly) output to stderr during builds (repros on non-Flutter apps). It is supppressed in xcode itself, but not when run from the command-line.

```
2023-11-10 10:44:58.031 xcodebuild[61115:1017566] [MT] DVTAssertions: Warning in /System/Volumes/Data/SWE/Apps/DT/BuildRoots/BuildRoot11/ActiveBuildRoot/Library/Caches/com.apple.xbs/Sources/IDEFrameworks/IDEFrameworks-22267/IDEFoundation/Provisioning/Capabilities Infrastructure/IDECapabilityQuerySelection.swift:103
Details:  createItemModels creation requirements should not create capability item model for a capability item model that already exists.
Function: createItemModels(for:itemModelSource:)
Thread:   <_NSMainThread: 0x6000027c0280>{number = 1, name = main}
Please file a bug at https://feedbackassistant.apple.com with this warning message and any useful information you can provide.
```

This suppresses this message from stderr in our macOS build logs.

Issue: flutter#135277  
Cherry-pick: flutter#139284

https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

*List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.*

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

* [CP] have `Java.version` return null if `java --version` fails or cannot be run (flutter#139698)

cherry-picks changes from flutter#139614 onto the stable channel

* [CP] Catch error for missing directory in `FontConfigManager` (flutter#138496) (flutter#139743)

Closes:
- flutter#138434

We will catch any errors while attempting to clear the temp directories that don't exist for the `FontConfigManager` class

---------

Co-authored-by: Kate Lovett <[email protected]>
Co-authored-by: Chris Bracken <[email protected]>
Co-authored-by: Andrew Kolos <[email protected]>
Co-authored-by: Elias Yishak <[email protected]>
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@andrewkolos andrewkolos deleted the fix-java-crash branch April 29, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flutter_tools] ProcessPackageException in Java.version

2 participants