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

feat: don't add a XCode prebuild action to invoke codegen anymore #679

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

atlj
Copy link
Collaborator

@atlj atlj commented Nov 8, 2024

Summary

We used to add an xcode prebuild script to invoke codegen each time the user built their example application. However, with the latest setup, the action just causes the build to fail immediately. This PR removes that script. With this removal, the user has to install pods each time they change their codegen specs.

Test plan

  1. Create a new library using crnl and make sure it supports the new architecture
  2. Install the pods and try building the example app using xcode
  3. Make sure the build doesn't fail immediately.

@satya164
Copy link
Member

satya164 commented Nov 8, 2024

So if I understand correctly, you'll need to run pod install after changing any line? It doesn't seem like a good DX.

However, with the latest setup, the action just causes the build to fail immediately

What's the reason for this? Is it not possible to fix it?

At the very least, is there any way to ensure that user knows that they need to run pod install first? Is it possible to fail the build in case of outdated generated code?

@atlj
Copy link
Collaborator Author

atlj commented Nov 8, 2024

What's the reason for this? Is it not possible to fix it?

It cannot find npx, which is needed to invoke bob's codegen build command. I also tried to use npm exec but it couldn't find npm too. I also tried to use React Native's WITH_ENVIRONMENT.sh, and it's able to find node but npx and npm are still missing. Also this step was causing a lot of issues for users. So I think it's the best to remove it.

At the very least, is there any way to ensure that user knows that they need to run pod install first?

The user actually always needed to call pod install if they changed the codegen specs, even with the previous setup where we didn't generate codegen for them. Codegen is invoked for iOS during pod install. So this won't affect the existing workflow.

Is it possible to fail the build in case of outdated generated code?

The build is going to fail if the user tries to build their example app with outdated codegen scaffold code.

@satya164
Copy link
Member

satya164 commented Nov 8, 2024

It cannot find npx, which is needed to invoke bob's codegen build command. I also tried to use npm exec but it couldn't find npm too.

Why do we need either of those? Can't we just check node_modules/.bin?

@atlj
Copy link
Collaborator Author

atlj commented Nov 8, 2024

Can't we just check node_modules/.bin?

I just checked and it's a great idea, however, I had lots of difficulties just trying to get it working on my machine:

First I tried to call bob directly from .bin without doing anything else and I got an error saying node not found. Then I tried to use React Native's script to inject the node binary into env but I kept getting the same error.

And also, looks like with XCode 16, we need to patch the file further to get the React Native vars:

diff --git a/example/ios/BuraktestExample.xcodeproj/xcshareddata/xcschemes/BuraktestExample.xcscheme b/example/ios/BuraktestExample.xcodeproj/xcshareddata/xcschemes/BuraktestExample.xcscheme
index 02d793e..b54af5f 100644
--- a/example/ios/BuraktestExample.xcodeproj/xcshareddata/xcschemes/BuraktestExample.xcscheme
+++ b/example/ios/BuraktestExample.xcodeproj/xcshareddata/xcschemes/BuraktestExample.xcscheme
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Scheme
    LastUpgradeVersion = "1210"
-   version = "1.3">
+   version = "1.7">
    <BuildAction
       parallelizeBuildables = "YES"
       buildImplicitDependencies = "YES">
@@ -11,6 +11,15 @@
             <ActionContent
                title = "Invoke Codegen"
                scriptText = "WITH_ENVIRONMENT=&quot;$REACT_NATIVE_PATH/scripts/xcode/with-environment.sh&quot;&#10;&#10;cd &quot;$WORKSPACE_PATH/../../../&quot; &amp;&amp; /bin/sh -c &quot;$WITH_ENVIRONMENT &apos;./node_modules/.bin/bob build --target codegen&apos;&quot;&#10;">
+               <EnvironmentBuildable>
+                  <BuildableReference
+                     BuildableIdentifier = "primary"
+                     BlueprintIdentifier = "13B07F861A680F5B00A75B9A"
+                     BuildableName = "BuraktestExample.app"
+                     BlueprintName = "BuraktestExample"
+                     ReferencedContainer = "container:BuraktestExample.xcodeproj">
+                  </BuildableReference>
+               </EnvironmentBuildable>
             </ActionContent>
          </ExecutionAction>
       </PreActions>

I think we can get this working with additional work, but this might be broken in the future as it relies on the XCode version, and the React Native version. So I'm not sure if it's worth the effort to update and maintain this script.

The main problem here is, that XCode gives you a clear env to run your scripts against. It isn't supposed to read your shell's environment and inject it into the script's running environment. And for that reason it gets pretty tricky to run any logic within XCode actions.

@satya164
Copy link
Member

satya164 commented Nov 8, 2024

How does React Native find node to build the JS bundle, launch the dev server etc. when building with XCode? What's different in our implementation?

@atlj
Copy link
Collaborator Author

atlj commented Nov 8, 2024

React Native uses a shell script to call various scripts. they inject the $NODE_BINARY from another script which uses an artifact from the pod install step.

@atlj atlj force-pushed the @atlj/remove-xcode-run-script-for-codegen branch from 44836fe to 624a60c Compare November 8, 2024 22:47
@satya164
Copy link
Member

satya164 commented Nov 8, 2024

Can't we use this NODE_BINARY as well?

@satya164
Copy link
Member

Can we add the script in Podfile instead?

@atlj atlj force-pushed the @atlj/remove-xcode-run-script-for-codegen branch 2 times, most recently from 5840521 to 076ec98 Compare November 15, 2024 18:05
@atlj atlj force-pushed the @atlj/remove-xcode-run-script-for-codegen branch from 076ec98 to 2ade5ef Compare November 15, 2024 19:23
@atlj
Copy link
Collaborator Author

atlj commented Nov 15, 2024

We tried to automate the process using https://github.com/CocoaPods/Xcodeproj, however, it's taking too much time to get everything right for all of the users. We're sunsetting this feature for now and we can return to this in the future.

@atlj atlj merged commit 8fc684a into main Nov 15, 2024
27 of 29 checks passed
@atlj atlj deleted the @atlj/remove-xcode-run-script-for-codegen branch November 15, 2024 19:25
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.

2 participants