-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
Add ability for PrebuiltWindowsApp to accept a zip archive. #103918
Conversation
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.
a few nits, but lgtm
); | ||
} | ||
|
||
if (!applicationBinary.existsSync()) { |
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.
Any reason not to check this before the check for .exe
?
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 was following the original code logic, but I do think it makes sense to check it before. Updated, thanks!
return null; | ||
} | ||
|
||
// Try to unpack as a 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.
Should this file end in .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.
Added the check, and updated test case for this.
final Directory tempDir = globals.fs.systemTempDirectory.createTempSync('flutter_app.'); | ||
try { | ||
globals.os.unzip(globals.fs.file(applicationBinary), tempDir); | ||
} on ProcessException { |
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 believe on Windows this uses package:archive
, so i doubt this will throw a ProcessException
. Looks like hte code throws ArchiveException
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.
Ah I didn't know that. Thanks for catching this!
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.
Still LGTM
This pull request is not suitable for automatic merging in its current state.
|
It will be used internally for supporting windows development workflow.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.