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] add metadata for path ops #39698

Merged
merged 3 commits into from Feb 28, 2023

Conversation

XilaiZhang
Copy link
Contributor

@XilaiZhang XilaiZhang commented Feb 17, 2023

Adds lib path ops .dylib to meta data file.

Currently, when building "//flutter/shell/testing:testing", it adds a duplicate entry of "impellerc" to the metadata file of artifacts.zip. I suspect there is a dependency of "//flutter/shell/testing:testing" on "//flutter/impeller/compiler:impellerc", but I traced a few hours and didn't track down why there is a duplicate entry of "impellerc".
Since the codesign handles duplicate entries already, I will leave the duplicate entry in the metadata file.

@christopherfujino
Copy link
Member

as I have no idea what's going on here or how gn works, I will defer approval to Godofredo :)

@christopherfujino
Copy link
Member

Since the codesign handles duplicate entries already, I will leave the duplicate entry in the metadata file.

Does it handle it by skipping repeat entries?

@XilaiZhang
Copy link
Contributor Author

Since the codesign handles duplicate entries already, I will leave the duplicate entry in the metadata file.

Does it handle it by skipping repeat entries?

Yeah the entries are compiled into a set https://github.com/flutter/cocoon/blob/main/codesign/lib/src/file_codesign_visitor.dart#L65 to eliminate duplicates

Copy link
Contributor

@godofredoc godofredoc left a comment

Choose a reason for hiding this comment

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

Does this change need a rebase?

@@ -16,6 +16,7 @@ generated_file("artifacts_entitlement_config") {
"//flutter/impeller/compiler:impellerc",
"//flutter/impeller/tessellator:tessellator_shared",
"//flutter/shell/testing:testing",
"//flutter/tools/path_ops:path_ops",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add this dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this dependency is needed to collect the metadata from tools/path_ops/BUILD.gn.

we defined a data_keys field in this target. So that if a matching data key is present in tools/path_ops/BUILD.gn, it is propagated and collected into the meta data file.

@XilaiZhang
Copy link
Contributor Author

Does this change need a rebase?

I created it recently, but it doesn't hurt to rebase again. I will rebase now.

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
5 participants