-
Notifications
You must be signed in to change notification settings - Fork 29.7k
fix: duplicated Intellij IDE message when running flutter doctor #129030
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: duplicated Intellij IDE message when running flutter doctor #129030
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| fileSystem.path.join(homeDirPath, 'Applications'), | ||
| ]; | ||
|
|
||
| bool checkForJetBrainsToolboxWrapper(String installPath) { |
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.
Some thoughts:
- it seems like this will only work on macOS. Is this issue affecting Windows and Linux users also?
- From reading the issue, it sounds like the reason we have duplicate entries is because JetBrains toolbox generates one small executable that points to the actual install. Should we not then be skipping the small executable during detection? Or would there be cases when that would lead to zero IntelliJ's being detected?
- Does this also affect Android Studio?
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.
- Yes, it works on macOS but won't affect Windows and Linux because it isolates in a IntelliJValidatorOnMac class specifically for macOS only. There are separate implementations for Window and Linux.
- Yes, when I was working on the issue, I found that the validator is first looking for all possible candidate path that contains Intellij IDE, then extract and validate information from
plistfile. Not sure about the zero Intellij scenario, I will try to verify it. - Android Studio is in another plugin validator so no affect on it.
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.
To answer 2. if there is no validator then it won't show anything form doctor.
andrewkolos
left a comment
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.
To expand on thought 2 from @christopherfujino's earlier comment, it's possible that we find the same installation once during our search in the /Applications directory and then again in our Spotlight search. I have not verified this.
The solution provided in this PR might be sufficient, but I think it might be more maintainable to instead deduplicate validators (i.e. those that have the same installPath) before returning the list rather than making an explicit check for toolbox. Let me know if you feel differently, @cychiang. This PR also needs test(s).
I'm not fully sure on this part, but I don't see duplicate results from the Spotlight search. I will try to verify it and see why it doesn't happened.
I will try to see if there is another solution is more maintainable. But I would like to keep the change small and minimal. Will do the test after that as well. |
|
@andrewkolos @christopherfujino
Root cause analysis:On L416 is trying to find candidates under certain paths: The default folder to store all installed applications from JetBrains Toolbox is The To sum up:
We don't see the similar behaviour in |
| // Remove duplicate validator based on its title | ||
| final Set<String> validatorTitles = <String>{}; | ||
| validators.removeWhere((DoctorValidator validator) { | ||
| return !validatorTitles.add(validator.title); | ||
| }); |
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.
Is it possible that a user's machine can have two installs with the same title (same version) that are both valid? Even if it's not possible now, it could be in the future.
Why not filter using install path instead?
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.
Hi! I was trying to find another time to open another issue for what I will mention because it turns out like a rabbit hole and more issues from this. But let me try to answer your concern first:
- why not filter using the install path instead: It creates another constraint to the install path since the JetBrains Toolbox supports changing the install path. For example, if we update the location to store the installed app and it contains a keyword that we want to filter, we might remove it accidentally.
- If there are multiple installs with the same title(either
IntelliJ IDEA Ultimate EditionorIntelliJ IDEA Community Edition) with the same install path and version: I would say it's a good scenario to remove it because they all provide the same information fordoctor. But it needs extra care to handle possible multi-version and so on.
I see this validator in another way. It's a validator to find all correct content and reduce it based on the version, install path and plugin path. Ideally, if we can parse the right install path from the shortcut app and update it, we can expect the same result from path-based and spotlight searches. It opens up the possibility of detecting multiple app versions and its plugins version.
However, I further dug into this issue and discovered that we need to update the logic to find the correct install and plugin paths. The plugin path is also not working, and the issue I would like to open is related to this one.
The result from the path search(toolbox) can get the correct plugin version, but the spotlight one is not. It's because of the L510 on intellij_validator. The shortcut app contains the JetBrainsToolboxApp key, and the value is the actual location of the app, the same as the spotlight search. It can form a correct path to the plugin's path.
However, the result from the spotlight search depends on the keyword in the plist, and that keyword doesn't exist in the shortcut app. It helps to differentiate the result from path-based and spotlight searches. But the logic to find the correct plugin path when JetBrainsToolboxApp doesn't exist in the plist is outdated, resulting in an incorrect plugin path.
To sum up:
We might want to improve the following things:
- Install and plugin path handling
- Reduce the result based on the candidate validator's title and version or other information before returning the list
The issue is growing too much compared to the original possible solution, so I would like to work on those things separately to keep the changes minimal.
It might be too long to read, and I would like your input @andrewkolos to continue the issue 😃
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.
Thanks for the write-up! I wouldn't be surprised that the logic around plugins doesn't work properly. I've worked on the AndroidStudioValidator recently, and I am pretty sure that class has similar issues with detecting the Dart and Flutter plugins. I agree that this is a separate issue, so please feel free to file that with the tool label.
Moving back to this issue/PR, I realize I was misremembering the issue that this is trying to fix. Indeed, we can't use the install path to filter here. In that case, should we go back to using the plist file or looking for Jetbrains Toolbox in the installation path? I think filtering on the title works, but the result could change based on the order in which we detect installations of IntelliJ.
andrewkolos
left a comment
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.
|
Hi @andrewkolos I updated the code and use Here is the short summary:
|
andrewkolos
left a comment
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.
Sorry for taking a while to get back to this PR. I have just one nitpick. Otherwise, LGTM.
packages/flutter_tools/lib/src/intellij/intellij_validator.dart
Outdated
Show resolved
Hide resolved
christopherfujino
left a comment
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.
LGTM
andrewkolos
left a comment
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.
LGTM
flutter/flutter@d55a7d8...65ff3cb 2023-07-08 [email protected] Roll Flutter Engine from 69eb8275ce47 to 189f823e7b41 (1 revision) (flutter/flutter#130201) 2023-07-08 [email protected] Roll Flutter Engine from d5a35b4650b1 to 69eb8275ce47 (1 revision) (flutter/flutter#130199) 2023-07-08 [email protected] Roll Flutter Engine from 9006633571bb to d5a35b4650b1 (1 revision) (flutter/flutter#130197) 2023-07-08 [email protected] Roll Flutter Engine from 4ca619166c4a to 9006633571bb (2 revisions) (flutter/flutter#130195) 2023-07-08 [email protected] Roll Flutter Engine from 13d9d84e8aba to 4ca619166c4a (2 revisions) (flutter/flutter#130191) 2023-07-08 [email protected] Roll Flutter Engine from 40a8732a5de0 to 13d9d84e8aba (2 revisions) (flutter/flutter#130189) 2023-07-08 [email protected] fix: duplicated Intellij IDE message when running flutter doctor (flutter/flutter#129030) 2023-07-08 [email protected] Remove unneeded configuration file (flutter/flutter#130183) 2023-07-08 [email protected] Roll Flutter Engine from 893ab3bf7bb9 to 40a8732a5de0 (1 revision) (flutter/flutter#130186) 2023-07-07 [email protected] Roll Flutter Engine from b39e6fe4b3bf to 893ab3bf7bb9 (1 revision) (flutter/flutter#130180) 2023-07-07 [email protected] Roll Flutter Engine from 7c83ea3e8542 to b39e6fe4b3bf (1 revision) (flutter/flutter#130176) 2023-07-07 [email protected] Add a threshold when comparing screen order for selectables. (flutter/flutter#130043) 2023-07-07 [email protected] Upgrade framework pub dependencies, roll engine with rolled dart sdk (flutter/flutter#130163) 2023-07-07 [email protected] Revert "[a11y] CupertinoSwitch On/Off labels" (flutter/flutter#130166) 2023-07-07 [email protected] Test that inspector does not hold objects. (flutter/flutter#130102) 2023-07-07 [email protected] Fix XCode download link (flutter/flutter#129795) 2023-07-07 [email protected] Roll Packages from 9bcf4bf to b61eea1 (1 revision) (flutter/flutter#130154) 2023-07-07 [email protected] (Raw)Autocomplete: Add optional [optionsViewOpenDirection] param (flutter/flutter#129802) 2023-07-07 [email protected] Tiny one space formatting fix (flutter/flutter#130053) 2023-07-07 [email protected] Roll Flutter Engine from 8aa2e6516af1 to 5ae09b8b4fa3 (7 revisions) (flutter/flutter#130150) 2023-07-07 [email protected] Add debugging for iOS startup test flakes (flutter/flutter#130099) 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 [email protected],[email protected],[email protected] 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
This PR fixes the duplicated message from
flutter doctorwhen installIntellij IDEfromJetBrains Toolbox.The solution is based on the #98276. Add a function to skip the creation of the validator for
Macwhen the key wordJetBrainsToolboxAppis in theinfo.plist.Before:

After:

fix #98276
Pre-launch Checklist
///).