Skip to content

Conversation

@mikelambert
Copy link
Contributor

This uses the build config to get the EXECUTABLE_PATH_NAME to use, instead of doing (scheme + '.app'). This matters for cases where Debug and Release configs produce differently-named apps, and lets it work within iOS.

It defaults to looking up the Debug config, since I believe run-ios only ever produces Debug builds in its current setup anyway.

Test plan (required)

My project is configured like this for debug:

BUNDLE_ID_SUFFIX = .debug;
PRODUCT_BUNDLE_IDENTIFIER = "com.DanceDeets.ios$(BUNDLE_ID_SUFFIX)";
PRODUCT_NAME = "DanceDeets Debug";

and this for release:

BUNDLE_ID_SUFFIX = "";
PRODUCT_BUNDLE_IDENTIFIER = "com.DanceDeets.ios$(BUNDLE_ID_SUFFIX)";
PRODUCT_NAME = DanceDeets;

I ran react-native run-ios in my project, and it produced:

Launching com.DanceDeets.ios.debug
com.DanceDeets.ios.debug: 20299

(whereas it would previously fail, trying to install DanceDeets.app (from the inferredScheme) after building DanceDeets Debug.app)

This uses the build config to get the EXECUTABLE_PATH_NAME to use, instead of doing (scheme + '.app'). This matters for cases where Debug and Release configs produce differently-named apps, and lets it work within iOS.

It defaults to looking up the Debug config, since I believe run-ios only ever produces Debug builds in its current setup anyway.
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @LearningDave and @grabbou to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 28, 2016
@javache
Copy link
Member

javache commented Oct 5, 2016

Does this solve the same problem as #10156? Could you work with @StevePottter to build a single solution?

@mikelambert
Copy link
Contributor Author

Your message had two broken links, but yes, this fixes the same issue as #10178 . @StevePotter, I believe my approach fixes this by being more intelligent, without the need of environment variable overrides. Steve, are you interested in trying my pull request to see if it fixes your use case too? Thanks.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 5, 2016
function getBuildConfig() {
const result = child_process.execFileSync(
'xcodebuild',
['-showBuildSettings', '-configuration', 'Debug'],
Copy link
Contributor

@StevePotter StevePotter Oct 12, 2016

Choose a reason for hiding this comment

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

-configuration Debug? What about production or other types? I would say that you should pass a scheme in and use that, like:
function getBuildConfig(scheme) {
const result = child_process.execFileSync(
'xcodebuild',
['-showBuildSettings', '-scheme', scheme],

@StevePotter
Copy link
Contributor

StevePotter commented Oct 12, 2016

Hi @mikelambert! Thanks for putting in the effort to solve this problem too. My approach gets the app file name by parsing build output. Not ideal, I know.

Your approach using xcodebuild -showBuildSettings is better. Honestly I didn't even know about it. But I've run multiple tests on your PR and can't get it to work. We have 4 schemes and 3 build configuration. The app name is either "Vydia Dev.app", "Vydia Beta.app", or "Vydia.app". The scheme chooses the build configuration and since react-native has the --scheme arg, I'd prefer to stick with schemes.

Your getBuildConfig() hardcodes the configuration to be "Debug". That's not going to work for me. I figured I could just pass the scheme instead. So I did: xcodebuild -showBuildSettings -scheme Dev. Here is the result.

As you can see, FULL_PRODUCT_NAME resolved to "Vydia.app", although when I actually build with the Dev scheme, the FULL_PRODUCT_NAME shown in the output is "Vydia Dev.app". If I run xcodebuild -showBuildSettings -configuration Debug, it works.

But again, we need to honor schemes. As far as I can tell, xcodebuild -showBuildSettings will not tell you the build configuration used. We could extract that information from the schemes, like in my case Vydia.xcodeproj/xcshareddata/xcschemes. Then pass the -configuration parameter instead of schemes to get the app name.

So in conclusion, right now it doesn't work in all cases and IMO isn't ready. Mine, which I consider an inferior approach, does work. Mine's #10156. Personally I'd like to see mine merged so it can get into the next release of RN. Then you can I can work together, if you'd like, to come up with an ideal solution.

+@javache

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 12, 2016
@mikelambert
Copy link
Contributor Author

I was unsure on the -configuration Debug as well. So to summarize:

  • The UI uses schemes, and schemes determine configurations, and configurations determine app names.
  • The commandline doesn't really support schemes, leaving us to control configurations. And unfortunately we can't know which configuration to use without parsing xcschemes (or some yet TBD commandline tool). So therefore commandline is out for now.

Thus leaving your working-hack as the quickest means to get things working for the next RN.

That all makes sense to me. I'll leave it up to RN devs on how they want to proceed, and their balance of "more logically-correct but as-yet-incomplete" vs "fix this quickly, with some brittle output parsing".

@StevePotter
Copy link
Contributor

Yes, you are right. The root problem is "xcodebuild -showBuildSettings -scheme xxx" is broken. The resolved app could be used by parsing the build configuration from the scheme file, which would be the workaround to keep your approach intact. Or you could parse build output, which was my approach. I have no idea where I can report this bug to Apple. We still have to work around it, but I'd like them to know.

@mikelambert
Copy link
Contributor Author

https://bugreport.apple.com is the correct place to report bugs to apple.

@facebook-github-bot
Copy link
Contributor

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @StevePotter as a potential reviewer. Could you take a look please or cc someone with more context?

@hramos
Copy link
Contributor

hramos commented Nov 10, 2016

@StevePotter @mikelambert how do you want to proceed here? @StevePotter mentioned having a separate PR that addresses this but that comment linked to this same PR.

@mikelambert
Copy link
Contributor Author

Yeah, the correct link to that PR was #10178 . It seems it got merged a few weeks ago, so I'll close this PR.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants