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

chore: fix grammar in Xcode 15 helper method name #39658

Closed
wants to merge 1 commit into from

Conversation

mikehardy
Copy link
Contributor

@mikehardy mikehardy commented Sep 26, 2023

Summary:

Thanks for working through Xcode 15 compatibility issues!

I reviewed the diff of the PR (#39474) that altered Xcode 15 settings for react-native release 0.72.5 and I noticed that

  • everything looked great (worth saying)
  • there was a grammatical error in another of the method names

Trivial errors but, as long as I was in there, thought I'd submit a PR

Changelog:

[IOS] [FIXED] - fix grammar in Xcode 15 helper method name

Test Plan:

CI should catch it of course, but in general I did a full grep for the name and changed everything / typical method name refactor

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. sony Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 26, 2023
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi @mikehardy, thanks for this pr. One of the grammar issue has been fixed already!
Can you rebase the PR on top of main, so we only have the other fix?

packages/react-native/scripts/cocoapods/utils.rb Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mikehardy mikehardy changed the title chore: fix typo / grammar in Xcode 15 helper method names chore: fix grammar in Xcode 15 helper method name Sep 27, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mikehardy
Copy link
Contributor Author

Hi @mikehardy, thanks for this pr. One of the grammar issue has been fixed already! Can you rebase the PR on top of main, so we only have the other fix?

Sure thing! rebased to current main as of this typing - edited the commit message to just mention the grammar change since typo already fixed

While I have your attention did you notice that the Xcode 15 linker project file changes are an "always add it" type of thing and thus accumulating on every run? That is every time I run pod install now it appears to add a new one, so the pbxproj is always perturbed with an extra set of linker commands instead of a "check and only add if not present" type pbxproj change

I'll look for an issue or report it but you may have seen this already. Luckily doesn't seem to effect the actual build (I tested it - if you have multiple instances of the same linker commands it is apparently okay even if untidy)

@cipolleschi
Copy link
Contributor

cipolleschi commented Sep 27, 2023

While I have your attention did you notice that the Xcode 15 linker project file changes are an "always add it" type of thing and thus accumulating on every run? That is every time I run pod install now it appears to add a new one, so the pbxproj is always perturbed with an extra set of linker commands instead of a "check and only add if not present" type pbxproj change

Yeah, people reported that but I'm struggling to understand why...
We call this function, implemented here, to add the flags only if they are missing.

So, unless I forgot how to read, it should add the flags only if it is not included already in the list of flags... 😕

Anyway, I have an alternative implementation in mind that might be more robust.

@mikehardy
Copy link
Contributor Author

mikehardy commented Sep 27, 2023

While I have your attention did you notice that the Xcode 15 linker project file changes are an "always add it" type of thing and thus accumulating on every run? That is every time I run pod install now it appears to add a new one, so the pbxproj is always perturbed with an extra set of linker commands instead of a "check and only add if not present" type pbxproj change

Yeah, people reported that but I'm struggling to understand why... We call this function, implemented here, to add the flags only if they are missing.

So, unless I forgot how to read, it should add the flags only if it is not included already in the list of flags... 😕

Anyway, I have an alternative implementation in mind that might be more robust.

I wonder if it is that they are slightly different? Note multi-line flags vs single line, as I take the state of the pbxproj from one invocation of pod install to two

mike@win-osxvm2:~/work/invertase/react-native-firebase (@mikehardy/dependency-updates *) % git diff tests/ios/testing.xcodeproj/
diff --git a/tests/ios/testing.xcodeproj/project.pbxproj b/tests/ios/testing.xcodeproj/project.pbxproj
index 10df3dc46..d93855942 100644
--- a/tests/ios/testing.xcodeproj/project.pbxproj
+++ b/tests/ios/testing.xcodeproj/project.pbxproj
@@ -517,6 +517,7 @@
                                        "-Wl",
                                        "-ld_classic",
                                        " ",
+                                       "-Wl -ld_classic ",
                                );
                                REACT_NATIVE_PATH = "${PODS_ROOT}/../../node_modules/react-native";
                                SDKROOT = iphoneos;
@@ -592,6 +593,7 @@
                                        "-Wl",
                                        "-ld_classic",
                                        " ",
+                                       "-Wl -ld_classic ",
                                );
                                REACT_NATIVE_PATH = "${PODS_ROOT}/../../node_modules/react-native";
                                SDKROOT = iphoneos;
mike@win-osxvm2:~/work/invertase/react-native-firebase (@mikehardy/dependency-updates *) % 

After that second run, I ran it a third time and received no extra diff. So perhaps it is idempotent after the second run (or following this multi-line hunch - "idempotent if the flags are exactly the same / on the same line already")?

Sorry to hijack this totally unrelated thread but I thought I could provide useful info since I reproduce it and have it open on my digital workbench right now

@github-actions
Copy link

This pull request was successfully merged by @mikehardy in fa87eaa.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Sep 27, 2023
@mikehardy mikehardy deleted the patch-1 branch September 27, 2023 23:19
yayvery pushed a commit to discord/react-native that referenced this pull request Oct 9, 2023
Summary:
Thanks for working through Xcode 15 compatibility issues!

I reviewed the diff of the PR (facebook#39474) that altered Xcode 15 settings for react-native release 0.72.5 and I noticed that

- everything looked great (worth saying)
- there was a grammatical error in another of the method names

Trivial errors but, as long as I was in there, thought I'd submit a PR

## Changelog:

[IOS] [FIXED] - fix grammar in Xcode 15 helper method name

Pull Request resolved: facebook#39658

Test Plan: CI should catch it of course, but in general I did a full grep for the name and changed everything / typical method name refactor

Reviewed By: cortinico

Differential Revision: D49641551

Pulled By: cipolleschi

fbshipit-source-id: d77d33bbd6941f039dd30766e1308d5c4c4a6ca8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants