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

Repro: xccurrentversion file not being picked up in rules_ios #2453

Conversation

thiagohmcruz
Copy link
Contributor

@thiagohmcruz thiagohmcruz commented Aug 9, 2023

If a versioned data model is set in resource_bundles Xcode is not picking up the correct current version (Model 2 in this example). To repro:

cd examples/rules_ios
bazel run --config=cache //:xcodeproj

Then note:
Screenshot 2023-08-09 at 9 26 13 AM

and once you open Xcode (just launch it, no additional action needed) Xcode apparently tries to "fix itself" and update the .xccurrentversion file to the wrong one (Model and not Model 2), causing a non-empty git status and the app to crash on launch since it will use a stale model.

Screenshot 2023-08-09 at 9 26 28 AM

Some facts I learned after some digging

  • rules_ios's _precompiled_apple_resource_bundle bundles these resources and it's possible there's something missing there an it's not an issue in rules_xcodeproj but I'm not 100% sure
  • Passing the .xccurrentversion files from ctx.attr.resources in rules_ios to datamodels "fixes" the issue above but triggers an undesired behaviour: datamodels being set triggers a rules_apple code path that places the compiled foo.app/bar.momd at the root of the .app/.xctest bundle duplicating the one rules_ios puts under foo.app/some_fmw.bundle/bar.momd
  • Passing the .xccurrentversion files from ctx.attr.resources in rules_ios to unprocessed without a "parent dir" "fixes" the issue above (since it will be processed here) but triggers an undesired behaviour: the .xccurrentversion files is added to the .app/.xctest bundle which is undesired and will fail the build if multiple models are bundled in the same framework.
  • If a space is not present in the model name the issue is gone (i.e. Model2 instead of Model 2) but I think this could be a false positive and simply "luck" that Xcode picks the right one in this case.

@thiagohmcruz
Copy link
Contributor Author

thiagohmcruz commented Aug 9, 2023

I guess one option is to create a rules_ios specific provider RulesiOSAppleResourceInfo (or something like that), propagate the .xccurrentversion file there and read in rules_xcodeproj if present? cc @brentleyjones I'd love any insight here since this is crashing the apps on launch for us and I'm trying to unblock folks wanting to use rules_xcodeproj 🙇

(although AppleResourceInfo is already propagated where it's needed so if we introduce a custom provider digging into the graph to find it might contain edge cases 🤔 )

@brentleyjones
Copy link
Contributor

I'll take a look now. I doubt a new provider is the answer.

@mattrobmattrob
Copy link
Contributor

I guess one option is to create a rules_ios specific provider RulesiOSAppleResourceInfo (or something like that), propagate the .xccurrentversion file there and read in rules_xcodeproj if present? cc @brentleyjones I'd love any insight here since this is crashing the apps on launch for us and I'm trying to unblock folks wanting to use rules_xcodeproj 🙇

(although AppleResourceInfo is already propagated where it's needed so if we introduce a custom provider digging into the graph to find it might contain edge cases 🤔 )

Do you have a repro branch in rules_ios or rules_xcodeproj that has the crashing app with your rules_ios changes to the normal AppleResourceInfo provider, @thiagohmcruz? I can take a look.

@thiagohmcruz
Copy link
Contributor Author

I guess one option is to create a rules_ios specific provider RulesiOSAppleResourceInfo (or something like that), propagate the .xccurrentversion file there and read in rules_xcodeproj if present? cc @brentleyjones I'd love any insight here since this is crashing the apps on launch for us and I'm trying to unblock folks wanting to use rules_xcodeproj 🙇
(although AppleResourceInfo is already propagated where it's needed so if we introduce a custom provider digging into the graph to find it might contain edge cases 🤔 )

Do you have a repro branch in rules_ios or rules_xcodeproj that has the crashing app with your rules_ios changes to the normal AppleResourceInfo provider, @thiagohmcruz? I can take a look.

@mattrobmattrob thx for taking a look too! I don't have a crashing app repro, our internal apps are crashing and the reason is because Xcode selected the wrong model as in the screenshot above. Then code paths relying on the new schema would hit a stale model. Based on my testing the change in git status I mentioned in the description was enough to know that an app could crash on launch since it's the wrong "current version".

Mind clarifying how a crashing app would help with the investigation here given that we know already that the wrong model is being selected (due to .xccurrentversion files not being picked up)? Just wanted to clarify and if it would help I could try to find a crashing app in rules_ios.

@mattrobmattrob
Copy link
Contributor

Mostly just trying to clarify a fix in rules_ios vs. rules_xcodeproj by checking out the fixes you put into rules_ios to see what's actually wrong with them (since I haven't tried to understand that yet).

…files

Signed-off-by: Brentley Jones <github@brentleyjones.com>
@thiagohmcruz
Copy link
Contributor Author

Mostly just trying to clarify a fix in rules_ios vs. rules_xcodeproj by checking out the fixes you put into rules_ios to see what's actually wrong with them (since I haven't tried to understand that yet).

@mattrobmattrob this branch, note that I reverted my previous attempts to fix this in rules_ios HEAD so this branch is based of this commit before I reverted everything here. The reason for the revert is because it was not the right approach as it would cause the issues I mentioned in this PR's description. From that branch run:

bazel run tests/ios/xcodeproj:Foo

open Foo.xcodeproj

and note how the wrong current version of the model is selected in Xcode. Then if you let the xccurrentversion files be propagated by undoing this you'll see that the issue is fixed but building

arch -arch x86_64 bazel build //tests/ios/unit-test/test-imports-app:TestImports-App --define=apple.experimental.tree_artifact_outputs=1

and inspecting the .app bundle shows duplicated Model.momd files (one if the issues described above in the description):

find bazel-bin/tests/ios/unit-test/test-imports-app/TestImports-App.app/ -name '*.momd'

bazel-bin/tests/ios/unit-test/test-imports-app/TestImports-App.app//CoreDataExample.bundle/Model.momd
bazel-bin/tests/ios/unit-test/test-imports-app/TestImports-App.app//Model.momd

(I can link to the code in rules_apple that puts the .momd at the root of the bundle as it wasn't there before, what is triggering it is the fact that datamodels was set in AppleResourceInfo in that branch as an attempt to fix this issue).

@thiagohmcruz
Copy link
Contributor Author

thiagohmcruz commented Aug 9, 2023

I see there's a new commit, thx @brentleyjones! Let me test this in my original internal repro. If it works do you want me to convert this to a proper PR to be landed?

@thiagohmcruz
Copy link
Contributor Author

With the latest commit here this now works as expected in my internal repro @brentleyjones. Thank you!

@brentleyjones
Copy link
Contributor

Do we think my change is correct? I would guess that it is, since any way that we get an .xccurrentversion file inside of .xcdatamodeld folder bundle, we:

  1. Shouldn't include it in the project navigator
  2. Should parse it to know how to set the values in the project

If everyone agrees, then yes, let's change this into a PR that talks about the fix (instead of all of the repro stuff) and we can merge it.

@thiagohmcruz
Copy link
Contributor Author

Do we think my change is correct? I would guess that it is, since any way that we get an .xccurrentversion file inside of .xcdatamodeld folder bundle, we:

  1. Shouldn't include it in the project navigator
  2. Should parse it to know how to set the values in the project

If everyone agrees, then yes, let's change this into a PR that talks about the fix (instead of all of the repro stuff) and we can merge it.

Yes to both 1. and 2..

There's a lot of info in this PR already so I'll create another one focused on the fix only.

brentleyjones added a commit that referenced this pull request Aug 9, 2023
Pulls @brentleyjones's commit to fix `.xccurrentversion` files not being
picked up when using `rules_ios`. Key change is to handle how
`rules_ios`'s `precompiled_apple_resource_bundle` rule exposes its
resources.

This was causing the wrong "current version" to be set in the generated
Xcode project. Adds a new `rules_ios` test case to exercise it.

Note: for future reference, original discussion happened
[here](#2453).

---------

Signed-off-by: Thiago Cruz <thiago.hmcruz@gmail.com>
Signed-off-by: Brentley Jones <github@brentleyjones.com>
Co-authored-by: Brentley Jones <github@brentleyjones.com>
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