-
Notifications
You must be signed in to change notification settings - Fork 27k
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 flutter build macos-framework command #105242
Conversation
Future<Set<DevelopmentArtifact>> get requiredArtifacts async => const <DevelopmentArtifact>{ | ||
DevelopmentArtifact.macOS, | ||
}; |
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.
nit:
Future<Set<DevelopmentArtifact>> get requiredArtifacts async => const <DevelopmentArtifact>{ | |
DevelopmentArtifact.macOS, | |
}; | |
Future<Set<DevelopmentArtifact>> get requiredArtifacts async => const <DevelopmentArtifact>{ | |
DevelopmentArtifact.macOS, | |
}; |
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.
dart-fmt did this 🙁 let me fix by hand.
/// contains the Flutter engine and framework code as well as plugins. It can | ||
/// be integrated into plain Xcode projects without using or other package | ||
/// managers. | ||
class BuildMacOSFrameworkCommand extends BuildFrameworkCommand { |
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 wanted to share more between this and BuildIOSFrameworkCommand
but almost every line is subtly different.
For example, the iOS assemble target only creates App.framework, but the macOS one creates App.framework and copies FlutterMacOS.framework, so a separate copy isn't needed. iOS publishes Flutter.xcframework, but macOS publishes FlutterMacOS.framework, which means the xcframework needs to be created. iOS has extra logic for building simulator frameworks, etc.
} | ||
|
||
final Status status = | ||
globals.logger.startProgress(' └─Moving to ${globals.fs.path.relative(modeDirectory.path)}'); |
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.
This doesn't look like it matches what the following try block is doing?
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.
You're right, I'll change to a printStatus
since I still want to convey the path.
modeDirectory.deleteSync(recursive: true); | ||
} | ||
|
||
if (boolArg('cocoapods') ?? false) { |
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 we make these boolArg(...)!
? (I believe) the only way this would error out is if a later refactor to the arg parsing results in this command no longer having a cocoapods
flag and this code wasn't updated, which would be a tool bug that we'd want to know about.
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.
@Jasguerrero I keep running into this pattern. Can we make boolArg
return a nonnull bool to simplify the code, and have it throw an exception (not toolExit
since it's a developer error) if the flag doesn't exist?
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.
that's a good idea
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.
Filed #105263 to track
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.
LGTM
Implement
flutter build macos-framework
to support basic add-to-app for macOS. This can be run from a normal macOS Flutter app, it does not implement the module template.I don't like the stray
Building App.framework for arm64..
logging but it's not coming from this PR, we can address that later.See also refactoring work in #105194
Fixes #104866
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.