-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Make macrobenchmarks buildable for macos #71760
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.
One more line needs removing, and that file should be added too, but otherwise LGTM.
@@ -101,9 +101,6 @@ unlinked_spec.ds | |||
|
|||
# macOS | |||
**/macos/Flutter/GeneratedPluginRegistrant.swift |
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 is wrong too. Only Flutter/ephemeral (which is in macos/.gitignore) is designed to be ignored on macOS. That way we can always add new files to it and control whether they are or are not checked in even for existing projects, to avoid issues I saw with mobile.
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 think that file needs to move to ephemeral then, it is generated:
//
// Generated file. Do not edit.
//
import FlutterMacOS
import Foundation
func RegisterGeneratedPlugins(registry: FlutterPluginRegistry) {
}
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 #71762, let's deal with that separately.
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.
For the reasons I gave in the bug, I am not confident that this PR will meet the started goal (having those be buildable on macOS) in all cases without fixing 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.
This project, at least, is buildable on macOS with just this change.
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.
LG
@@ -0,0 +1,2 @@ | |||
#include "Pods/Target Support Files/Pods-Runner/Pods-Runner.debug.xcconfig" | |||
#include "ephemeral/Flutter-Generated.xcconfig" |
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 don't really know how desktop works so I might be asking dumb things. What's the macos/Flutter/ephemeral/flutter_export_environment.sh
file for? I don't see it referenced anywhere.
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 think I just never special cased that code to not generate the script for macOS. It would (someday) be used for macOS add-to-app though. It's unused right now.
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 ok. good to know. I was trying to figure out where it plugged in :D
Description
Flutter-Debug.xcconfig
andFlutter-Release.xcconfig
are supposed to be checked into the git repo. https://github.com/flutter/flutter/pull/48251/files should not have added these gitignore exceptions.You can see these are correctly checked in for flutter_gallery:
https://github.com/flutter/flutter/tree/89ef88c64f4d4b4fe81d44174de0e0abb442817f/dev/integration_tests/flutter_gallery/macos/Flutter
Also, actually build it and let CocoaPods do its thing.