Plugin tool updates #6054
Plugin tool updates #6054
Conversation
This function replicates the same behavior as the --strip-components=N flag used in the `tar` command.
…platform selector for all other versions.
Code on line 124 performs the same action, just with a 'rebuildCache' check and directory creation should be handled by our file extraction to extra/empty dir's aren't created needlessly.
Update calling methods. This and other like methods have the Future signature removed since they all executed using await in any case. Return values are still 'int' to support existing checks.
…nd is not found.
Update variable names to make intentions more apparent.
The reference to the accumulator is already available and doesn't need to be passed in each time. Bit easier to read.
|
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 <#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
<#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
<#6054 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANSKNWZNY7DZUW4DXOOS3TVBSCA7ANCNFSM5RPQJCFQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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 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): |
|
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
<#6054 (comment)>,
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 #6052 in case anyone else wants to take it over. |
3 participants
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 #6052
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.