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

[gn + codesign] mac code sign configuration for FlutterMacOS.framework.zip #35707

Merged
merged 12 commits into from
Feb 17, 2023

Conversation

XilaiZhang
Copy link
Contributor

@XilaiZhang XilaiZhang commented Aug 25, 2022

context: this is related to recent change at https://github.com/flutter/engine/pull/35673/files , where the FlutterMacOS.framework.zip is now built through the global generator scripts.

This commit embed code signing configurations in the global generator script for FlutterMacOS.framework.zip.

@XilaiZhang XilaiZhang changed the title [codesign] macos config [gn + codesign] mac code sign configuration for FlutterMacOS.framework.zip Aug 25, 2022
)

# Zip FlutterMacOS.framework.
subprocess.check_call([
Copy link
Member

Choose a reason for hiding this comment

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

Is this the resolution for #35623 that was reverted here #35680, because it was expected to not have entitlements? I'm not sure how these without_entitlements files work.

https://github.com/flutter/flutter/blob/fefb2b00bab8548e66aa09849dbd51c4b9c357d1/dev/conductor/core/lib/src/codesign.dart#L188

@christopherfujino

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry didn't realize there is an engine PR review on Thursday. These without_entiltements and 'entitlements.txt` are just text files embedded in the zip artifacts, not going to affect the correctness of anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inspecting locally, looks like the symlinks are dropped in the build process. Current assumption is based on #35673.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ready for review?

@XilaiZhang XilaiZhang requested a review from godofredoc August 25, 2022 21:42
@XilaiZhang
Copy link
Contributor Author

Yeah I will convert this to a draft for now, and come back when #35673 is ready

@XilaiZhang XilaiZhang marked this pull request as draft August 25, 2022 21:44
@cbracken cbracken self-requested a review October 1, 2022 01:33
@Hixie
Copy link
Contributor

Hixie commented Jan 17, 2023

@XilaiZhang Is this still on your radar? Looks like #35673 has landed.

@XilaiZhang
Copy link
Contributor Author

Thanks for the reminder! revisiting this pr and refreshing memories.

@XilaiZhang
Copy link
Contributor Author

rebuilt all the sub-builds and verified that the metadata files still work as expected for global generators. reopening this pull requests.
However, across multiple builds it seems like all the symlinks in FlutterMacOS.framework are still dropped. I think this is a known issue and @godofredoc might be the expert to provide insights on the symlink drops. Thank you!

@chinmaygarde
Copy link
Member

Is there an issue filed for preserving symlinks in frameworks that are archived? In the meantime, marking this as WIP.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Jan 19, 2023
@jmagman
Copy link
Member

jmagman commented Jan 19, 2023

Is there an issue filed for preserving symlinks in frameworks that are archived? In the meantime, marking this as WIP.

I don't think there are checks in the engine, but the last time this was an issue I wrote tests in the framework to validate the expected framework bundle structure after it's unzipped:
https://github.com/flutter/flutter/blob/c5ceff11dd5226c3daaea867ed0c60ce4245d67a/packages/flutter_tools/test/host_cross_arch.shard/macos_content_validation_test.dart#L136

@XilaiZhang
Copy link
Contributor Author

Is there an issue filed for preserving symlinks in frameworks that are archived? In the meantime, marking this as WIP.

sorry just to clarify, the engine artifacts in production have the correct symlink. It was just that when I build and test the artifact locally through gn the symlinks aren't there (probably because the symlinks are added through recipe). yeah for local build and test the issue is being discussed in flutter/flutter#107674. Thanks for triaging!

@chinmaygarde chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label Feb 16, 2023
@XilaiZhang XilaiZhang requested a review from jmagman February 17, 2023 01:25
@@ -152,6 +157,30 @@ def process_framework(dst, args, fat_framework, fat_framework_binary):
],
cwd=dst)

macos_filepath_with_entitlements = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Should lines 160 to 170 be moved to line 151 and remove the zipping subprocess? as it is we are zipping twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated to zip only once. Yeah I remember this artifact was historically unzipped twice (https://github.com/christopherfujino/codesign.py/blob/master/codesign.py#L151) for some reasons, but verified just now and it looks there we can do a single zip/unzip.

Also it's interesting that if i double click to extract the artifact, the symlinks are not preserved. But if i use the subprocess command "unzip.. -d .." then the symlinks are preserved in the extracted file.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Thanks!

@XilaiZhang XilaiZhang added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 17, 2023
@auto-submit auto-submit bot merged commit 9c6993e into flutter:main Feb 17, 2023
godofredoc added a commit that referenced this pull request Feb 18, 2023
@godofredoc
Copy link
Contributor

Reverting because this change broke some lints: https://ci.chromium.org/p/flutter/builders/prod/Linux%20Android%20clang-tidy/2079

auto-submit bot pushed a commit that referenced this pull request Feb 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 18, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 18, 2023
…121023)

* 9c6993e52 [gn + codesign] mac code sign configuration for FlutterMacOS.framework.zip (flutter/engine#35707)

* 2fdce9a96 Revert "[gn + codesign] mac code sign configuration for FlutterMacOS.framework.zip (#35707)" (flutter/engine#39735)
@jmagman
Copy link
Member

jmagman commented Feb 21, 2023

Reverting because this change broke some lints: https://ci.chromium.org/p/flutter/builders/prod/Linux%20Android%20clang-tidy/2079

Hm, why didn't Linux Android clang-tidy run in presubmit checks https://github.com/flutter/engine/pull/35707/checks? I think it should have since this change included a python file:

engine/.ci.yaml

Line 254 in 3aa112c

- "**.py" # Run pylint on the fastest clang-tidy builder.

@jmagman
Copy link
Member

jmagman commented Feb 21, 2023

Hm, why didn't Linux Android clang-tidy run in presubmit checks

Filed flutter/flutter#121160

XilaiZhang added a commit to XilaiZhang/engine that referenced this pull request Mar 22, 2023
…k.zip (flutter#35707)

* embed codesign config in macos framework

* format files

* update path

* y flag for outer layer zip

* simplify structure

* format files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants