Skip to content

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented May 22, 2023

Fixes #124252, finishing work on the umbrella tracking issue, #126126.

Essentially, after this PR, no (non-test) code should be be referencing/invoking the java home or binary paths.

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.

@andrewkolos andrewkolos added platform-android Android applications specifically tool Affects the "flutter" command-line tool. See also t: labels. t: gradle "flutter build" and "flutter run" on Android labels May 22, 2023
@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems c: contributor-productivity Team-specific productivity, code health, technical debt. labels May 22, 2023
@andrewkolos andrewkolos marked this pull request as ready for review May 23, 2023 19:45
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be outside the scope of the pr but I think _java should provide a Version object so that this type of conversion does not happen in multiple places slightly differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to use the Version object here by using the Version.withText constructor to preserve the full version string (e.g. 'openjdk 19.0.2 2023-01-17').

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment appears to be true given https://github.com/flutter/flutter/blob/e8f5a51f79fc5dd2f8de38b4d9f504d09dca8197/packages/flutter_tools/lib/src/flutter_cache.dart#L411-L413, though I am not sure if it should. I've created #127848 to track as a follow-up and added a TODO comment linking to it

@github-actions github-actions bot removed c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels May 30, 2023
andrewkolos and others added 15 commits May 31, 2023 14:24
…7975)

flutter/engine@ee53c04...8e67cc3

2023-05-31 skia-flutter-autoroll@skia.org Roll Skia from f475d4f5e080 to cb883f64681b (6 revisions) (flutter/engine#42452)
2023-05-31 kjlubick@users.noreply.github.com Fix bugprone-unchecked-optional-access errors in image_generator_apng (flutter/engine#42450)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
)

flutter/engine@8e67cc3...b9860dc

2023-05-31 kjlubick@users.noreply.github.com Replace use of Skia's private GrRectanizer with a copy of the equivalent code (flutter/engine#42430)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@github-actions github-actions bot added the engine flutter/engine related. See also e: labels. label May 31, 2023
@github-actions github-actions bot removed the engine flutter/engine related. See also e: labels. label May 31, 2023
@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 1, 2023
@auto-submit auto-submit bot merged commit 06e2b18 into flutter:master Jun 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2023
@andrewkolos andrewkolos deleted the java-finding branch June 15, 2023 01:02
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App platform-android Android applications specifically t: gradle "flutter build" and "flutter run" on Android

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The logic used to find a JDK in various tool operations is unclear and confusing

3 participants