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

[TIMOB-25576] Ensuring usage of macos bundled core utilities and shells when adding to iOS buildPhases #9639

Merged
merged 8 commits into from Jan 20, 2018

Conversation

rlustemberg
Copy link
Contributor

@rlustemberg rlustemberg commented Nov 30, 2017

JIRA: https://jira.appcelerator.org/browse/TIMOB-25576

Make sure we are using bundled Unix utilities on iOS build phases

@build
Copy link
Contributor

build commented Nov 30, 2017

Messages
📖

🎉 Another contribution from our awesome community member, rlustemberg! Thanks again for helping us make Titanium SDK better. 👍

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@sgtcoolguy sgtcoolguy added the ios label Nov 30, 2017
@hansemannn hansemannn changed the title AC-5412 Ensuring usage of macos bundled core utilities and shells when adding to iOS buildPhases [TIMOB-25576] Ensuring usage of macos bundled core utilities and shells when adding to iOS buildPhases Nov 30, 2017
@@ -3177,7 +3177,7 @@ iOSBuilder.prototype.createXcodeProject = function createXcodeProject(next) {
outputPaths: [],
runOnlyForDeploymentPostprocessing: 0,
shellPath: '/bin/sh',
shellScript: '"cp -rf \\"$PROJECT_DIR/ArchiveStaging\\"/ \\"$TARGET_BUILD_DIR/$PRODUCT_NAME.app/\\""',
shellScript: '"/bin/cp -rf \\"$PROJECT_DIR/ArchiveStaging\\"/ \\"$TARGET_BUILD_DIR/$PRODUCT_NAME.app/\\""',
Copy link
Collaborator

@hansemannn hansemannn Dec 1, 2017

Choose a reason for hiding this comment

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

There is a possible performance issue with standalone commands. Apple even recommends to not use them and gives some intersting benchmarking-tests. I don't this issue is present for many users, but @janvennemann or @sgtcoolguy may have some more insight here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was unaware of a performance difference. Being that it's called a "shellScript", I assumed Xcode was just spawning the command in a shell in which case I don't see why /bin/cp would be significantly slower than cp. In the Apple docs, they use the example of the echo command which writes to stdout and perhaps that's where the speed issue is. cp doesn't output anything unless the verbose flag is set. So, I'm not terribly concerned with performance.

I guess I'm more concerned why /bin isn't in your PATH.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we are talking about one iteration of the command here, i think we can neglect the performance impact (if there even is one). We are talking about a few ms at most according to @rlustemberg benches.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let‘s take it for 7.0.1!

@rlustemberg
Copy link
Contributor Author

rlustemberg commented Dec 1, 2017

In my case I've uninstalled the homebrew coreutils package and didn't need the changes in the end. It took me some time to figure out and thought it a good idea to save the problem to someone else , providing a PR. For the record,I've just run some simple benchmarks using 'time' and the difference was negligible,

Richards-iMac-2:benchmarks richard$ time cp -rf orig/ArchiveStaging/ dest/
real 0m0.087s
user 0m0.003s
sys 0m0.068s
Richards-iMac-2:benchmarks richard$ time /bin/cp -rf orig/ArchiveStaging/ dest/
real 0m0.075s
user 0m0.003s
sys 0m0.069s

The results were more or less the same every time, sometimes the standalone command would be faster, sometimes slower. The difference was a few percentile points , definitely not orders of magnitude such as Apple's example of the echo command. It was tested on a computer with AFS and ssd drive. I would expect the difference to be much less on machines with hard drives and HFS (if someone still works on them nowadays)

@janvennemann janvennemann added this to the 7.0.1 milestone Dec 4, 2017
@hansemannn
Copy link
Collaborator

Backport: #9663

Copy link
Contributor

@mukherjee2 mukherjee2 left a comment

Choose a reason for hiding this comment

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

Approved. Passed FR.

@hansemannn hansemannn modified the milestones: 7.0.1, 7.0.2 Jan 20, 2018
@hansemannn hansemannn merged commit 91ba00a into tidev:master Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants