Allow downloading of desktop embedding artifacts #30648
Conversation
|
This should not land until #30153 to prevent this from leaking into stable. |
goderbauer
added
a: desktop
|
|
||
| _makeFilesExecutable(dir); | ||
|
|
||
| // TODO(jonahwilliams): clean this logic up to better handle different framework names. |
There was a problem hiding this comment.
How about something like:
final List<String> frameworkNames = ['Flutter', 'FlutterMacOS'];
for (frameworkName in frameworkNames) {
final File frameworkZip = fs.file(fs.path.join(dir.path, '$frameworkName.framework.zip'));
...
}
rather than duplicating the code.
| framework.createSync(); | ||
| os.unzip(frameworkZip, framework); | ||
| } | ||
| final File desktopFrameworkZip = fs.file(fs.path.join(dir.path, 'FlutterMacOS.framework.zip')); |
There was a problem hiding this comment.
Since I'm not familiar with the overall structure of this code: why does the macOS framework need to be specified/handled in two different places? It's both here and in the artifact list later. The iOS code doesn't seem to have the same structure (it's not obvious to me why Flutter.framework has special handling here, but it's not in the list below like the macOS version is.)
There was a problem hiding this comment.
Its a bit of a historical artifact (har-har), I can't quite remove it yet but eventually this will be gone. TLDR: I am scared to touch it
jonahwilliams
force-pushed
the
download_desktop_artifacts
branch
from
e6d4573 to
4425191
Compare
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I cloned this to try it out (with the fix for the check below); --macos is working for me, but --linux and --windows appear to be no-ops; nothing prints out, and I can't find any .dll or .so files in bin/cache, nor the client wrapper source. Is it filtering by my host platform?
| } | ||
| if (argResults['web']) { | ||
| requiredArtifacts.add(DevelopmentArtifact.web); | ||
| if (FlutterVersion.instance.isStable) { |
There was a problem hiding this comment.
This check is reversed.
There was a problem hiding this comment.
Perhaps I should add a test...
…/flutter into download_desktop_artifacts
|
Right now this is filtering by host platform, so macos artifacts are only downloaded for mac. If that isn't correct, I can remove that restriction. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with nits. (Caveat: I'm not familiar with the Dart testing infrastructure yet, so my review of the tests is probably useful. Up to you if you want a second reviewer for that part.)
* master: (209 commits) Allow mouse hover to only respond to some mouse events but not all. (flutter#30886) Fix issue 23527: Exception: RenderViewport exceeded its maximum number of layout cycles (flutter#30809) Keep hover annotation layers in sync with the mouse detector. (flutter#30829) Use identical instead of '==' in a constant expression. (flutter#30921) Add toggle for debugProfileWidgetBuilds (flutter#30867) Revert "Manual engine roll with disabled service authentication codes (flutter#30919)" (flutter#30930) Manual engine roll with disabled service authentication codes (flutter#30919) Baseline Aligned Row (flutter#30746) [Material] Fix showDialog crasher caused by old contexts (flutter#30754) Let `sliver.dart` `_createErrorWidget` work with other Widgets (flutter#30880) Add more dialog doc cross-reference (flutter#30887) Allow downloading of desktop embedding artifacts (flutter#30648) CupertinoDatePicker initialDateTime accounts for minuteInterval (flutter#30862) add golden tests for CupertinoDatePicker (flutter#30828) Simplify toImage future handling (flutter#30876) Fixed Table flex column layout error flutter#30437 (flutter#30470) Fix iTunes Transporter quirk (flutter#30883) Bump Android build tools to 28.0.3 in Dockerfile (flutter#30832) Update the upload key which seems to have trouble for some reason (flutter#30871) Check for invalid elevations (flutter#30215) ...
4 participants
Description
Adds desktop embedding shells to cache logic, and allows downloading via the precache command.
Related Issues
Fixes #30663
Tests
I added the following tests:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?