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

[flutter_tools] Remove Version.unknown #124771

Merged

Conversation

andrewkolos
Copy link
Contributor

Fixes #124756 by removing the concept of Version.unknown.

Version fields that needed the ability to represent an unknown version have been made nullable. Assigning null to them represents an unknown version.

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.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 13, 2023
@andrewkolos andrewkolos marked this pull request as ready for review April 13, 2023 20:01
@@ -245,7 +246,17 @@ class AndroidStudio implements Comparable<AndroidStudio> {
}
AndroidStudio? newest;
for (final AndroidStudio studio in studios.where((AndroidStudio s) => s.isValid)) {
if (newest == null || studio.compareTo(newest) > 0) {
newest ??= studio;
Copy link
Member

Choose a reason for hiding this comment

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

Is this functionality unit tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests. I ran in my local master branch to (hopefully) verify that no behavior was changed.

Copy link
Member

@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.

Overall LGTM, please just add unit tests of comparing versions

@andrewkolos
Copy link
Contributor Author

Fun fact: Version.parse parses '7unknown' as 7.0.0 🙃

if (studio.version != null && newest.version == null) {
newest = studio;
} else if (studio.version == null && newest.version == null &&
studio.directory.compareTo(newest.directory) > 0) {

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, yeesh, so we're just alphabetically comparing these strings?

Copy link
Member

Choose a reason for hiding this comment

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

Let me file a tracking issue about investigating whether we should really be doing this.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a TODO with a link to #124880

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, yeesh, so we're just alphabetically comparing these strings?

Only if 1) both installations have equivalent versions or 2) neither installation have known versions. I wonder what the original inspiration for this was...perhaps it was just a way keep ordering consistent even if the discovery logic changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I might be slightly incorrect here. Let me re-examine the original code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, reading this more, I can see this is a last-resort fallback.

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 think I have the logic right. I added another test

}
return AndroidStudio(configuredStudioPath,
configured: configuredStudio);
return AndroidStudio(correctedConfiguredStudioPath,
Copy link
Member

Choose a reason for hiding this comment

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

I know we used to do this, but should we not toolexit in the case where correctedConfiguredStudioPath does not exist on disk? This most likely means they either made a mistake setting their config value, or forgot they set that and then deleted Android Studio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this and a test

@@ -39,15 +39,16 @@ final RegExp _dotHomeStudioVersionMatcher =
// See https://github.com/flutter/flutter/issues/124252.
String? get javaPath => globals.androidStudio?.javaPath;

class AndroidStudio implements Comparable<AndroidStudio> {
class AndroidStudio {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you move the comparison logic out of AndroidStudio.compareTo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My instinct is to never implement Comparable unless the semantics of comparison are immediately obvious (i.e. do not require reading documentation or code). This makes sense for things like numbers and dates. It makes less sense for things like software installs where the version (or lack of it) and the directory name are involved.

I would consider consider the implementation if we had a need to compare AndroidStudio installs in multiple places in code, but we are only doing it in one place now.

I don't mind either way

Copy link
Member

Choose a reason for hiding this comment

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

Ahh ok, if we were only comparing this once, this makes sense.

Copy link
Member

@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 Apr 14, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 14, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 14, 2023

auto label is removed for flutter/flutter, pr: 124771, due to - The status or check suite Linux build_tests_1_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 14, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 14, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 14, 2023

auto label is removed for flutter/flutter, pr: 124771, due to - The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 15, 2023
@auto-submit auto-submit bot merged commit 68ec71f into flutter:master Apr 15, 2023
123 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 16, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Apr 16, 2023
flutter/flutter@00171b0...50171bb

2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from da0805a9cf03 to 4a998b73a2df (5 revisions) (flutter/flutter#124944)
2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from feb476b7b991 to da0805a9cf03 (4 revisions) (flutter/flutter#124933)
2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from ee66f0d6f2c9 to feb476b7b991 (2 revisions) (flutter/flutter#124929)
2023-04-15 jonahwilliams@google.com Revert "[framework] use shader tiling instead of repeated calls to drawImage" (flutter/flutter#124640)
2023-04-15 jonahwilliams@google.com [cupertino] improve cupertino picker performance by using at most one opacity layer (flutter/flutter#124719)
2023-04-15 andrewrkolos@gmail.com [flutter_tools] Remove `Version.unknown` (flutter/flutter#124771)
2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 41c1ea9a8283 to ee66f0d6f2c9 (2 revisions) (flutter/flutter#124923)

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

To file a bug in Packages: 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
exaby73 pushed a commit to exaby73/flutter_nevercode that referenced this pull request Apr 17, 2023
Fixes flutter#124756 by removing the concept of `Version.unknown`.

`Version` fields that needed the ability to represent an unknown version have been made nullable. Assigning `null` to them represents an unknown version.
@andrewkolos andrewkolos deleted the remove-version-unknown-null-object branch April 21, 2023 19:53
gmackall pushed a commit to gmackall/flutter that referenced this pull request May 4, 2023
Fixes flutter#124756 by removing the concept of `Version.unknown`.

`Version` fields that needed the ability to represent an unknown version have been made nullable. Assigning `null` to them represents an unknown version.
gmackall pushed a commit to gmackall/flutter that referenced this pull request May 5, 2023
Fixes flutter#124756 by removing the concept of `Version.unknown`.

`Version` fields that needed the ability to represent an unknown version have been made nullable. Assigning `null` to them represents an unknown version.
XilaiZhang pushed a commit that referenced this pull request May 15, 2023
…d Studio bundled Java version is detected (and 3 others) (#126093)

This is a cherry pick of the following four PRs (the first one depends
on changes made in the latter three).
1. #125836
2. #124771
3. #123034
4. #123644

For the first PR, 
Original issue: #122376
Cherry-pick issue: #125693

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

## Pre-launch Checklist

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

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

<!-- Links -->
[Contributor Guide]:
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/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

---------

Co-authored-by: Reid Baker <reidbaker@google.com>
Co-authored-by: Mitchell Goodwin <58190796+MitchellGoodwin@users.noreply.github.com>
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Co-authored-by: Victoria Ashworth <vashworth@google.com>
Co-authored-by: Jackson Gardner <jacksongardner@google.com>
Co-authored-by: Rydmike <m.rydstrom@gmail.com>
Co-authored-by: keyonghan <54558023+keyonghan@users.noreply.github.com>
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>
Co-authored-by: Taha Tesser <tessertaha@gmail.com>
Co-authored-by: Ben Konyi <bkonyi@google.com>
Co-authored-by: engine-flutter-autoroll <engine-flutter-autoroll@skia.org>
Co-authored-by: hellohuanlin <41930132+hellohuanlin@users.noreply.github.com>
Co-authored-by: Danny Tuppeny <danny@tuppeny.com>
Co-authored-by: Chris Bracken <chris@bracken.jp>
Co-authored-by: Kate Lovett <katelovett@google.com>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: Elias Yishak <42216813+eliasyishak@users.noreply.github.com>
Co-authored-by: Christopher Fujino <fujino@google.com>
Co-authored-by: Andrew Kolos <andrewrkolos@gmail.com>
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
…#3723)

flutter/flutter@00171b0...50171bb

2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from da0805a9cf03 to 4a998b73a2df (5 revisions) (flutter/flutter#124944)
2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from feb476b7b991 to da0805a9cf03 (4 revisions) (flutter/flutter#124933)
2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from ee66f0d6f2c9 to feb476b7b991 (2 revisions) (flutter/flutter#124929)
2023-04-15 jonahwilliams@google.com Revert "[framework] use shader tiling instead of repeated calls to drawImage" (flutter/flutter#124640)
2023-04-15 jonahwilliams@google.com [cupertino] improve cupertino picker performance by using at most one opacity layer (flutter/flutter#124719)
2023-04-15 andrewrkolos@gmail.com [flutter_tools] Remove `Version.unknown` (flutter/flutter#124771)
2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 41c1ea9a8283 to ee66f0d6f2c9 (2 revisions) (flutter/flutter#124923)

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

To file a bug in Packages: 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
FlutterSu pushed a commit to flutter-rosita/flutter-rosita that referenced this pull request Sep 14, 2023
…d Studio bundled Java version is detected (and 3 others) (flutter#126093)

This is a cherry pick of the following four PRs (the first one depends
on changes made in the latter three).
1. flutter#125836
2. flutter#124771
3. flutter#123034
4. flutter#123644

For the first PR,
Original issue: flutter#122376
Cherry-pick issue: flutter#125693

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

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

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

<!-- Links -->
[Contributor Guide]:
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/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

---------

Co-authored-by: Reid Baker <reidbaker@google.com>
Co-authored-by: Mitchell Goodwin <58190796+MitchellGoodwin@users.noreply.github.com>
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Co-authored-by: Victoria Ashworth <vashworth@google.com>
Co-authored-by: Jackson Gardner <jacksongardner@google.com>
Co-authored-by: Rydmike <m.rydstrom@gmail.com>
Co-authored-by: keyonghan <54558023+keyonghan@users.noreply.github.com>
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>
Co-authored-by: Taha Tesser <tessertaha@gmail.com>
Co-authored-by: Ben Konyi <bkonyi@google.com>
Co-authored-by: engine-flutter-autoroll <engine-flutter-autoroll@skia.org>
Co-authored-by: hellohuanlin <41930132+hellohuanlin@users.noreply.github.com>
Co-authored-by: Danny Tuppeny <danny@tuppeny.com>
Co-authored-by: Chris Bracken <chris@bracken.jp>
Co-authored-by: Kate Lovett <katelovett@google.com>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: Elias Yishak <42216813+eliasyishak@users.noreply.github.com>
Co-authored-by: Christopher Fujino <fujino@google.com>
Co-authored-by: Andrew Kolos <andrewrkolos@gmail.com>
FlutterSu pushed a commit to flutter-rosita/flutter-rosita that referenced this pull request Dec 27, 2023
…d Studio bundled Java version is detected (and 3 others) (flutter#126093)

This is a cherry pick of the following four PRs (the first one depends
on changes made in the latter three).
1. flutter#125836
2. flutter#124771
3. flutter#123034
4. flutter#123644

For the first PR,
Original issue: flutter#122376
Cherry-pick issue: flutter#125693

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

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

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

<!-- Links -->
[Contributor Guide]:
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/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

---------

Co-authored-by: Reid Baker <reidbaker@google.com>
Co-authored-by: Mitchell Goodwin <58190796+MitchellGoodwin@users.noreply.github.com>
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Co-authored-by: Victoria Ashworth <vashworth@google.com>
Co-authored-by: Jackson Gardner <jacksongardner@google.com>
Co-authored-by: Rydmike <m.rydstrom@gmail.com>
Co-authored-by: keyonghan <54558023+keyonghan@users.noreply.github.com>
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>
Co-authored-by: Taha Tesser <tessertaha@gmail.com>
Co-authored-by: Ben Konyi <bkonyi@google.com>
Co-authored-by: engine-flutter-autoroll <engine-flutter-autoroll@skia.org>
Co-authored-by: hellohuanlin <41930132+hellohuanlin@users.noreply.github.com>
Co-authored-by: Danny Tuppeny <danny@tuppeny.com>
Co-authored-by: Chris Bracken <chris@bracken.jp>
Co-authored-by: Kate Lovett <katelovett@google.com>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: Elias Yishak <42216813+eliasyishak@users.noreply.github.com>
Co-authored-by: Christopher Fujino <fujino@google.com>
Co-authored-by: Andrew Kolos <andrewrkolos@gmail.com>
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
2 participants