Update getBuildPath to grab from xcodebuild -showBuildSettings comman… #19305
Conversation
…d so that custom per-configuration build paths are respected.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@kimihiro64 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
@TheSavior Tagging you as you've reviewed contribution to this recently. I've looking over the failed tests and they don't seem to be related to the change I made, could be wrong though. |
| @@ -27,7 +28,14 @@ const getBuildPath = function(configuration = 'Debug', appName, isDevice) { | |||
| device = 'iphonesimulator'; | |||
| } | |||
|
|
|||
| return `build/Build/Products/${configuration}-${device}/${appName}.app`; | |||
| let command = `xcodebuild -${isWorkspace ? 'workspace' : 'project'} ${projectName} -scheme ${scheme} -configuration ${configuration} -derivedDataPath ${derivedDataPath} -sdk ${device} -showBuildSettings`; | |||
| let settings = child_process.execSync(command); | |||
There was a problem hiding this comment.
can you change this to spawnSync so we don't run the risk of shell injection?
There was a problem hiding this comment.
I will look into that, wasn't thinking about it as I was assuming user's going to be entering valid scheme and such- can't user self-shell injection be a feature? :p
There was a problem hiding this comment.
I think this code isn't very vulnerable in its current state but if someone were to modify how one of the interpolated variables is set, they could be unintentionally opening an injection hole.
| let settings = child_process.execSync(command); | ||
| let output = settings.toString('utf8'); | ||
|
|
||
| let path = /target React:(.|\n)*?target (.*?):(.|\n)*? BUILT_PRODUCTS_DIR = (.*?)\n/m.exec(output); |
There was a problem hiding this comment.
most of the capturing groups can be cleanly removed so there's just the one that's actually used e.g. [\s\S]*? That said, maybe capture the target name too and print it in a helpful console log particularly because you mentioned this could be a source of bugs if the intended target doesn't always follow React? At least this way people have more visibility into what your heuristic matched.
| let settings = child_process.execSync(command); | ||
| let output = settings.toString('utf8'); | ||
|
|
||
| let path = /target React:(.|\n)*?target (.*?):(.|\n)*? BUILT_PRODUCTS_DIR = (.*?)\n/m.exec(output); |
There was a problem hiding this comment.
seems like the names path and match should be swapped i.e. the regex result is a match and it contains the path we care about
|
We have since moved this code into https://github.com/react-native-community/react-native-cli. Please reopen the same PR in the repo there. Thank you! |
…d so that custom per-configuration build paths are respected. Uses BUILT_PRODUCTS_DIR for the target immediately after React target in the output- it would be nicer if we could use the target name to ensure we're pulling the exact directory, but I can't get the target name without a ton of work reading other xcode project files and plists. However, every correctly configured scheme /should/ have the main target directly after React (if this is not always the case, I'll update where it will pull the correct target for the scheme).
Thank you for sending the PR! We appreciate you spending the time to work on these changes.
Help us understand your motivation by explaining why you decided to make this change.
Fixes #19300
Test Plan
Tested run-ios with multiple schemes/configurations (Debug, DebugProd, DebugQa in my case).
Related PRs
Release Notes
[CLI] [BUGFIX] [runIOS] - Updates run-ios to use xcodebuild's BUILT_PRODUCTS_DIR when installing applications.