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

Use bundle and add proper logging for pod installation #617

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

kmagiera
Copy link
Member

@kmagiera kmagiera commented Oct 13, 2024

This is a draft as it is build on top of #615

This PR refactors the way we install pods in the IDE. The main objective is to optionally use bundle command. This is similar to the changes proposed in #304 but apart from using bundle for the diagnostics check, we also use it when installing (which is a mechanism that wasn't in place when #304 was created).

The second main change that we're making is that we trigger pod installation when clean build is requested. Before that wasn't happening.

On top of that, we are including pod command output in the iOS build log. This will help diagnose potential problems better and will also provide some feedback for the scenarios when installation take very long.

Finally, we are fixing the startup message component such that it resets long wait flag and we are also adding a more prominent message "open logs" for the native build phase. This way it should be easier to spot for the users that they can see the build output in case the build takes long:
image

How Has This Been Tested:

  1. Run clean build in on of the test apps for iOS. See cocoapods in the log output
  2. Run RN 76 example on iOS to see bundle being used there instead of pod install (check logs)
  3. When building on iOS, see that the "open logs" text appears in the startup message and you can click it to open the logs output.

Copy link

vercel bot commented Oct 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 14, 2024 9:19pm

Base automatically changed from kmagiera/deps-redo to main October 14, 2024 15:35
@@ -155,7 +155,7 @@ export class DependencyManager implements Disposable, DependencyManagerInterface
return true;
}

public async installPods(cancelToken: CancelToken) {
public async installPods(buildOutpuChannel: OutputChannel, cancelToken: CancelToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo:
buildOutpuChannel -> buildOutputChannel


try {
await cancelToken.adapt(commandInIosDir("pod install"));
lineReader(process).onLineRead((line) => buildOutpuChannel.appendLine(line));
Copy link
Contributor

Choose a reason for hiding this comment

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

typo:
buildOutpuChannel -> buildOutputChannel

Copy link
Contributor

@p-malecki p-malecki left a comment

Choose a reason for hiding this comment

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

Looks good, I left inline comment about typo.

@kmagiera kmagiera merged commit ddd99f8 into main Oct 14, 2024
3 checks passed
@kmagiera kmagiera deleted the kmagiera/bundle-pods-and-logs branch October 14, 2024 21:19
kmagiera added a commit that referenced this pull request Oct 15, 2024
In #617 we added an option for the IDE to use bundle command instead of
pods directly as it is now recommended in new react native templates and
setups. However, for bundle to work properly, we need to `bundle
install` first. This was missed in that PR as I was testing a project
where bundle install has already run.

This PR changes the pod installation command to: `bundle install &&
bundle exec pod install` as recommended by react native docs. For this
to work we pass `shell` option to the command which allows for joining
the commands.

### How Has This Been Tested: 
1. Delete rn 75 project and checkout a fresh copy
2. Open the fresh RN 75 project with the IDE
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