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

fix(macCatalyst): construct correct path for macCatalyst build #2312

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

mikehardy
Copy link
Contributor

Summary:

Hi there! I have a demo script I use to test ios + android builds in both debug+release on rc candidates to make sure react-native-firebase will work with upcoming releases

I'm testing the 0.74-rc series now

It also tests macCatalyst builds, and since that's a pretty niche build setup I frequently find little issues.

I believe I found a little issue here, where after the build (which did work...) there was no ability to launch the macCatalyst app because the path construction for the macCatalyst build was very subtly incorrect


Related but not completely related:

I actually have a separate issue with macCatalyst builds in that it is impossible without a hacky-patch to target macCatalyst from the command line but I'm not sure what to do with that. Explanation here in the patch


Test Plan:

This was tested via patch-package applying this patch to the build code for the cli-platform-package package in node_modules in an automated build harness that I have here (this is pointing to the 0.74 rc testing branch: https://github.com/mikehardy/rnfbdemo/blob/rn74/make-demo.sh)

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

Copy link
Collaborator

@szymonrybczak szymonrybczak left a comment

Choose a reason for hiding this comment

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

Thank you for fixing! Ah, as always small details introduces bugs :D I've introduced while doing a refactor #2234 😅 Will make sure that it will land in the next RC 🙌

@szymonrybczak
Copy link
Collaborator

I actually have a separate issue with macCatalyst builds in that it is impossible without a hacky-patch to target macCatalyst from the command line but I'm not sure what to do with that. Explanation here in the patch

Could you please create a separate issue with this problem? 🙏 I would love to help:)

@mikehardy
Copy link
Contributor Author

I actually have a separate issue with macCatalyst builds in that it is impossible without a hacky-patch to target macCatalyst from the command line but I'm not sure what to do with that. Explanation here in the patch

Could you please create a separate issue with this problem? 🙏 I would love to help:)

Sure - I've got the issue well identified at least but....I'm not even sure how to handle it so it would be nice to have a collaborator. Normally the solution is obvious and I'll just post a PR but right now what I've got is a hack. Anyway, I'll make an issue.

Thanks for shepherding this fix in this PR though though :-)

@cortinico
Copy link
Member

@szymonrybczak can we get this merged and shipped in a CLI point release, otherwise this won't make it inside 0.74

@szymonrybczak szymonrybczak merged commit 0b78b71 into main Mar 4, 2024
10 checks passed
@szymonrybczak szymonrybczak deleted the mikehardy-macCatalyst-buildpath-fix branch March 4, 2024 11:10
@cortinico
Copy link
Member

@szymonrybczak this change is not included in any release of the CLI. Can we release it in a point release so we can include inside RC4?

@thymikee
Copy link
Member

Released in v13.6.2, thanks!

@cortinico
Copy link
Member

Released in v13.6.2, thanks!

@thymikee @szymonrybczak can one of you bump the CLI against the release branch and against main?

@thymikee
Copy link
Member

@cortinico released main as 14.0.0-alpha.0 ✨

@szymonrybczak
Copy link
Collaborator

Released in v13.6.2, thanks!

@thymikee @szymonrybczak can one of you bump the CLI against the release branch and against main?

0.74-stable -> facebook/react-native#43414

main -> facebook/react-native#43415

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.

4 participants