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

Collect all .xcdatamodeld instead of only .xccurrentversion #750

Closed
wants to merge 6 commits into from

Conversation

thiagohmcruz
Copy link
Contributor

@thiagohmcruz thiagohmcruz commented Aug 2, 2023

Follow up to #747 and #748.

Noticed that some files were not being copied into the bundle correctly if we collect only the .xccurrentversion in the datamodels attribute. So collecting all files under the .xcdatamodeld directory instead.

Specifically, from this PR's branch, you can see the issue by building this target

bazel build tests/ios/app:AppTestOnly --define=apple.experimental.tree_artifact_outputs=1

and noticing how there's an additional .momd file being placed at the root of the app bundle (it happens with .xctest bundles too). This file is a duplicate of the .momd that is placed under the SomeDependency.bundle directory.

This PR specifically ensures we're packing the bundles the same way as before while exposing data model files in the datamodels attribute of the AppleResourceInfo provider.

Before:
Screenshot 2023-08-03 at 3 07 27 PM

After:
Screenshot 2023-08-03 at 3 08 38 PM

@thiagohmcruz thiagohmcruz force-pushed the thiago/fix-xccurrentversion-collection branch from d859471 to 9659a12 Compare August 2, 2023 21:00
@thiagohmcruz thiagohmcruz force-pushed the thiago/fix-xccurrentversion-collection branch from 9659a12 to f3a7c95 Compare August 3, 2023 19:09
@thiagohmcruz thiagohmcruz marked this pull request as ready for review August 3, 2023 19:12
Copy link
Collaborator

@justinseanmartin justinseanmartin left a comment

Choose a reason for hiding this comment

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

Still looks reasonable to me, as long as it seems to clear up the reported issue and still fixes the original issue this was attempting to fix.

Comment on lines 162 to +174
# Create a file for bundletool to know what files to copy
# https://github.com/bazelbuild/rules_apple/blob/d29df97b9652e0442ebf21f1bc0e04921b584f76/tools/bundletool/bundletool_experimental.py#L29-L46
#
# ps: removing `.momd` files from here since they'll be collected in `AppleResourceInfo` => `datamodels`
# below and added under the `.bundle` directory like other resources. This is so data model files
# are visible to other rules looking for information in the `AppleResourceInfo` provider. See this
# PR for more context: https://github.com/bazel-ios/rules_ios/pull/750
control_files = [
f
for f in control_files
if hasattr(f, "src") and not f.src.endswith(".momd")
]

Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: Feels like the comments are out of order now and could be a bit more terse. Also, bikeshedding a bit, I think the PR information can generally be left out, as it can be traced back from git commit metadata when useful, but usually doesn't add much value and reduced readability.

Suggested change
# Create a file for bundletool to know what files to copy
# https://github.com/bazelbuild/rules_apple/blob/d29df97b9652e0442ebf21f1bc0e04921b584f76/tools/bundletool/bundletool_experimental.py#L29-L46
#
# ps: removing `.momd` files from here since they'll be collected in `AppleResourceInfo` => `datamodels`
# below and added under the `.bundle` directory like other resources. This is so data model files
# are visible to other rules looking for information in the `AppleResourceInfo` provider. See this
# PR for more context: https://github.com/bazel-ios/rules_ios/pull/750
control_files = [
f
for f in control_files
if hasattr(f, "src") and not f.src.endswith(".momd")
]
# `*.momd` files will be collected in `AppleResourceInfo` below and added under the `.bundle` directory like other resources.
control_files = [
f
for f in control_files
if hasattr(f, "src") and not f.src.endswith(".momd")
]
# Create a file for bundletool to know what files to copy
# https://github.com/bazelbuild/rules_apple/blob/d29df97b9652e0442ebf21f1bc0e04921b584f76/tools/bundletool/bundletool_experimental.py#L29-L46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me, def being more verbose here than necessary. These changes are still not right though and I'll continue to investigate. The failures in the latest run show me that something is missing since a test that was passing before is not anymore.

Will make the changes once I root cause that.

@thiagohmcruz
Copy link
Contributor Author

... this is not right. This is changing things in the bundle still and that's not the intent. This test

bazel test //tests/ios/frameworks/core-data-resource-bundle:CoreDataExampleTests

was 🟢 before and is 🔴 in this PR.

I'll revisit my approach for this .xccurrentversion story and propose a better change or consider fixing this directly in rules_xcodeproj. Closing for now and sorry for the noise 🤦‍♂️.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants