flutter-intellij
flutter-intellij copied to clipboard
flutter
Plugin tool updates
This PR brings the project closer to normalizing the tool across platforms.
Previously, this plugin used OS specific calls like rm, mv, curl to complete. These calls have been replaced with platform-agnostic Dart. Additionally, file pathing has been normalized to use Dart's path library.
Issues fixed: Addresses https://github.com/flutter/flutter-intellij/issues/6052
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 the Flutter Style Guide recently, and have followed its advice.
- [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.
I read through these changes and see nothing that obviously needs to be changed. I'm going to be OOO today but I'll finish my review tomorrow. I did not see an entry in our AUTHORS file. If that's intentional, fine, but if you want to be listed please add that.
The checks will find some problems that have already been fixed in other PRs. Until #6047 is merged, every check fails. Even then, there will be problems with the 2022.1 build and unit tests. This PR is not affected by those checks, so you can ignore them. Those will be fixed by #5985.
Recently, admins changed the repo to require commits to be signed. It looks like yours are not, so you're going to have to jump through some hoops to set that up. If you want to get a head-start on that, the instructions are here. When I did it, the instructions were not particularly well-written and it took longer to figure out the commands to type than it did to type them. Hopefully that has changed :)
No worries and no rush on this one. I've found a 'bug' in the pathing that
I'm gonna push later and there's a few open questions I thought might be
answered by people more familiar. One such question: I don't remember how
path redirects work with files. i.e. there's are two files, artifacts and
resources that have a single line that points to the directory above
them. I think that's something unique to Piper but I can't remember
exactly and I'm not sure what the preferred way of resolving that should
be.
I'd forgotten about the AUTHORS file, I'll update that as well, thanks for the reminder!
On Thu, Mar 24, 2022 at 11:21 AM stevemessick @.***> wrote:
I read through these changes and see nothing that obviously needs to be changed. I'm going to be OOO today but I'll finish my review tomorrow. I did not see an entry in our AUTHORS file. If that's intentional, fine, but if you want to be listed please add that.
The checks will find some problems that have already been fixed in other PRs. Until #6047 https://github.com/flutter/flutter-intellij/pull/6047 is merged, every check fails. Even then, there will be problems with the 2022.1 build and unit tests. This PR is not affected by those checks, so you can ignore them. Those will be fixed by #5985 https://github.com/flutter/flutter-intellij/pull/5985.
Recently, admins changed the repo to require commits to be signed. It looks like yours are not, so you're going to have to jump through some hoops to set that up. If you want to get a head-start on that, the instructions are here https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification. When I did it, the instructions were not particularly well-written and it took longer to figure out the commands to type than it did to type them. Hopefully that has changed :)
— Reply to this email directly, view it on GitHub https://github.com/flutter/flutter-intellij/pull/6054#issuecomment-1077748703, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANSKNWZNY7DZUW4DXOOS3TVBSCA7ANCNFSM5RPQJCFQ . You are receiving this because you authored the thread.Message ID: @.***>
there are two files,
artifactsandresourcesthat have a single line that points to the directory above them.
This is one of those "things are the way they are because they got that way" situations. You've read the CONTRIBUTING.md file, right? The Mac/Linux instructions used to be much the same as the Windows instructions. However, our unit tests stopped working and the only way I could find to fix them was to convert the project to a Gradle project, without IntelliJ or Android Studio sources. Unless you needed to debug unit tests you could still continue using the file-based project (the *.iml file), as long as the two dirs you mention were still available. So I kept them at top level and added symbolic links to the flutter-idea module. It turned out that was great for Windows, once I figured out how to check out a project with symlinks, because I ran into confusing problems with the Gradle project on Windows. And, if I need to debug Android Studio-specific code I use the file-based project because I wasn't able to load the Gradle project as a module into the Android Studio source-based project.
If you've wondered why there are so many problem in the Project Structure Editor, it is because one *.iml file serves for both IntelliJ source-based, and Android Studio source-based projects. I really should make separate files for them, just haven't had time.
I patched in this PR and ran it on my Mac. I had a couple problems. First, it downloads android-studio-ide-2021.1.1.8-linux.tar.gz even though android-studio-2021.1.1.8-linux.tar.gz (no "-ide") is already in artifacts. And then it unpacks both of them. It should only do one. Also, when it unpacks the one with "-ide" it does not move the contents of artifacts/android-studio/android-studio to artifacts/android-studio so the build fails. This all may be due to the pathing issue you mentioned earlier.
For context, the complexity with names is due to Android Studio and IntelliJ changing the names of their download files. We didn't want to have to rename the files so we made the tool adapt. It may be time to simplify that. More complexity was added because, until recently, I had very slow internet (took ~1 hour to d/l Android Studio), so I added a check for the Mac version to avoid another d/l.
Also, while I have no problem changing the log messages to be platform-agnostic, I would like to preserve the equivalent of "unzip" and "tar". I wasn't sure the artifacts had been unpacked until I checked the directories. It isn't necessary to mention that directories are being created; that's implied by the unpacking.
Here's my log file for reference (the 2022.1 build is known to fail until another PR gets merged): original.log
Awesome, thanks for that. I'm going to continue to see if I can't get this all to unpack 'right' and get it back to you. I'll keep it in sync with the HEAD and ping you when it's in a better spot. If you want to close the PR for now, that's fine too. This is just my own lightbulb fixing moment while I was trying to dig into the unit testing issues with the flutter plugin.
Definitely keeping the 'unzip' and 'untar' as separate functions. Similar methods though.
On Fri, Mar 25, 2022 at 2:13 PM stevemessick @.***> wrote:
I patched in this PR and ran it on my Mac. I had a couple problems. First, it downloads android-studio-ide-2021.1.1.8-linux.tar.gz even though android-studio-2021.1.1.8-linux.tar.gz (no "-ide") is already in artifacts. And then it unpacks both of them. It should only do one. Also, when it unpacks the one with "-ide" it does not move the contents of artifacts/android-studio/android-studio to artifacts/android-studio so the build fails. This all may be due to the pathing issue you mentioned earlier.
For context, the complexity with names is due to Android Studio and IntelliJ changing the names of their download files. We didn't want to have to rename the files so we made the tool adapt. It may be time to simplify that. More complexity was added because, until recently, I had very slow internet (took ~1 hour to d/l Android Studio), so I added a check for the Mac version to avoid another d/l.
Also, while I have no problem changing the log messages to be platform-agnostic, I would like to preserve the equivalent of "unzip" and "tar". I wasn't sure the artifacts had been unpacked until I checked the directories. It isn't necessary to mention that directories are being created; that's implied by the unpacking.
Here's my log file for reference (the 2022.1 build is known to fail until another PR gets merged): original.log https://github.com/flutter/flutter-intellij/files/8352874/original.log
— Reply to this email directly, view it on GitHub https://github.com/flutter/flutter-intellij/pull/6054#issuecomment-1079280279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANSKNUKSUUKKB3LRZ7FSF3VBX637ANCNFSM5RPQJCFQ . You are receiving this because you authored the thread.Message ID: @.***>
Great, looking forward to finishing the review. We'll keep this one open.
@firephreek Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue.
@firephreek Thanks for the contribution! I'm going to close this since it hasn't been touched for a while, to get it off our review queue. Please don't hesitate to reopen it if you have a chance to get back to it. Thanks! In the meantime I'll mention this PR in https://github.com/flutter/flutter-intellij/issues/6052 in case anyone else wants to take it over.