-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Unify engineDirs and toolsDir in Cache of Flutter Tools. #7644
Conversation
/cc @tvolkert Test? |
The real fix here is to only download the files we need when we need them. See #6491 |
'android-arm', | ||
'android-arm-profile', | ||
'android-arm-release', | ||
'android-x64', |
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.
What happened to android-x64
and android-x86
?
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.
Done.
]; | ||
|
||
List<List<String>> get _iosBinaryDirs => <List<String>>[ | ||
<String>['ios', 'android-arm/artifacts.zip'], |
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.
Not android
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.
Done.
List<List<String>> get _iosBinaryDirs => <List<String>>[ | ||
<String>['ios', 'android-arm/artifacts.zip'], | ||
<String>['ios-profile', 'ios-profile/artifacts.zip'], | ||
<String>['ios-release', 'os-release/artifacts.zip'], |
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.
ios
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.
Done.
} | ||
|
||
List<List<String>> get _osxToolsDirs => <List<String>>[ | ||
List<List<String>> get _osxBinaryDirs => <List<String>>[ |
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.
Since the cache directory basename is repeated in the artifact portion of every one of these entries, we should strip it from the artifact portion and just update download()
accordingly.
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.
I made the change. However, I don't really like it. It makes the download logic more complicated for very little benefit.
Maybe I misunderstood what you meant?
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.
I didn't notice there were cases with subfolders in the cache dir, which makes the login in download()
less clear than I had envisioned...
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.
Reverted.
422959e
to
43a6cc5
Compare
The zip files downloaded from cloud storage are not clearly separated into engine artifacts and tools anyways. Also, because of this unclear separation, we were downloading `linux-x64/artifacts.zip` twice before.
7d218a3
to
d718515
Compare
Re:Test - To properly test this, a new abstraction for the downloader would be required. I think that's outside the scope of the PR. |
LGTM |
The zip files downloaded from cloud storage are not clearly separated into engine artifacts and tools anyways.
Also, because of this unclear separation, we were downloading
linux-x64/artifacts.zip
twice before.