Skip to content
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

Regenerate FlutterMacOS.xcframework when sources of dependencies change #51396

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

vashworth
Copy link
Contributor

Fixes flutter/flutter#144716.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Comment on lines 320 to 325
":copy_dylib",
":copy_framework_headers",
":copy_framework_icu",
":copy_framework_info_plist",
":copy_framework_module_map",
":copy_license",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since _generate_symlinks_and_verify_framework_module deps on _generate_symlinks which deps on this same list, it seems like maybe the deps aren't the source of the problem. Maybe sources = [ $root_build_dir/$_flutter_framework_name.stamp ] would make this re-run if the framework is rebuilt?

@zanderso what's the gn idiomatic way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the right way to do this is to leave this deps list unchanged, but to make _generate_symlinks a public_deps of _generate_symlinks_and_verify_framework_module. (I think that's all that should be needed, but making the deps of _generate_symlinks be public_deps in addition would be the next thing to try.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately setting deps to public_deps for both _generate_symlinks and _generate_symlinks_and_verify_framework_module didn't work, perhaps because _generate_symlinks_and_verify_framework_module is a shared_library?

Looking at how we do this for iOS, we have the flutter_framework depend on a group universal_flutter_framework, which then has a dependency on the shared_library (copy_and_verify_framework_module) and the other deps

group("universal_flutter_framework") {
visibility = [ ":*" ]
deps = [
":copy_and_verify_framework_module",
":copy_dylib",
":copy_framework_icu",
":copy_framework_info_plist",
":copy_framework_module_map",
":copy_framework_privacy_manifest",
":copy_license",
]
if (is_asan) {
deps += [ ":copy_asan_runtime_dylib" ]
}
}
action("flutter_framework") {
script = "//flutter/sky/tools/create_xcframework.py"
outputs = [ "$root_out_dir/Flutter.xcframework" ]
args = [
"--frameworks",
rebase_path("$_flutter_framework_dir"),
"--name",
"Flutter",
"--location",
rebase_path("$root_out_dir"),
]
deps = [ ":universal_flutter_framework" ]
}

I could add a group to the macOS version to mimic it, also perhaps instead of the deps from _generate_symlinks, just use _generate_symlinks as the dep. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmagman @zanderso friendly ping

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shucks. The sematics of GN deps and when thy block and when they propagate are certainly confusing. If there's a working pattern on the macOS side, then mimicking that here makes sense to me.

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM to whatever @zanderso approves.

@vashworth vashworth requested a review from zanderso March 19, 2024 20:07
@vashworth vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 19, 2024
@auto-submit auto-submit bot merged commit c9f3e78 into flutter:main Mar 19, 2024
24 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 19, 2024
…145429)

flutter/engine@bd3a924...dce639a

2024-03-19 skia-flutter-autoroll@skia.org Roll Skia from f67715b35713 to 0b3bd42131fe (12 revisions) (flutter/engine#51530)
2024-03-19 15619084+vashworth@users.noreply.github.com Regenerate FlutterMacOS.xcframework when sources of dependencies change (flutter/engine#51396)
2024-03-19 skia-flutter-autoroll@skia.org Roll Dart SDK from 7c64e31f4c3e to 7885a3fa6bfd (1 revision) (flutter/engine#51528)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: desktop autosubmit Merge PR when tree becomes green via auto submit App platform-macos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FlutterMacOS.xcframework does not get updated during build
3 participants