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(ios): fixed debug log server issues (TIMOB-27074) #10975

Closed
wants to merge 13 commits into from
Closed

fix(ios): fixed debug log server issues (TIMOB-27074) #10975

wants to merge 13 commits into from

Conversation

cb1kenobi
Copy link
Contributor

fix(ios): Fixed bug where the log server was being started when the iOS app was quitting instead of stopping it.
fix(ios): Changed the log server port test to use 127.0.0.1 instead of localhost to avoid bad dns lookup.
fix(ios): Fixed log server port test timeout to properly handle the timeout. (TIMOB-27074)

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

fix(ios): Fixed bug where the log server was being started when the iOS app was quitting instead of stopping it.
fix(ios): Changed the log server port test to use 127.0.0.1 instead of localhost to avoid bad dns lookup.
fix(ios): Fixed log server port test timeout to properly handle the timeout. (TIMOB-27074)
@build build added this to the 8.2.0 milestone Jun 18, 2019
@build build requested a review from a team June 18, 2019 19:08
@build
Copy link
Contributor

build commented Jun 18, 2019

Fails
🚫

iphone/cli/commands/_build.js#L2412 - iphone/cli/commands/_build.js line 2412 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L2422 - iphone/cli/commands/_build.js line 2422 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L2425 - iphone/cli/commands/_build.js line 2425 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L3814 - iphone/cli/commands/_build.js line 3814 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L3897 - iphone/cli/commands/_build.js line 3897 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L3903 - iphone/cli/commands/_build.js line 3903 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L3906 - iphone/cli/commands/_build.js line 3906 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L3941 - iphone/cli/commands/_build.js line 3941 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L4111 - iphone/cli/commands/_build.js line 4111 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L4112 - iphone/cli/commands/_build.js line 4112 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L4113 - iphone/cli/commands/_build.js line 4113 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L4273 - iphone/cli/commands/_build.js line 4273 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

🚫

iphone/cli/commands/_build.js#L4285 - iphone/cli/commands/_build.js line 4285 – Do not access Object.prototype method 'hasOwnProperty' from target object. (no-prototype-builtins)

Warnings
⚠️

Commit 5ec724e895738432c2ca326ae84b69cfb505f110 has a message "fix(ios): fixed debug log server issues (TIMOB-27074)
fix(ios): Fixed bug where the log server was being started when the iOS app was quitting instead of stopping it.
fix(ios): Changed the log server port test to use 127.0.0.1 instead of localhost to avoid bad dns lookup.
fix(ios): Fixed log server port test timeout to properly handle the timeout. (TIMOB-27074)" giving 1 warnings:

  • body must have leading blank line
⚠️

Commit bdc5769e403b8c3235a9caa3dbceb96d8071e5eb has a message "refactor(ios): Reworked the flow for detecting the log server port to be amazing." giving 3 errors:

  • header must not be longer than 72 characters, current length is 81
  • subject must not be sentence-case, start-case, pascal-case, upper-case
  • subject may not end with full stop
⚠️

Commit 16871d6075b9f9b680982b1a7c6216665ff6a20f has a message "fix(ios): Removed log server client check and just randomly pick an available port. TIMOB-27074" giving 2 errors:

  • header must not be longer than 72 characters, current length is 95
  • subject must not be sentence-case, start-case, pascal-case, upper-case
⚠️

iphone/cli/commands/_build.js#L5846 - iphone/cli/commands/_build.js line 5846 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)

⚠️

iphone/cli/commands/_build.js#L6373 - iphone/cli/commands/build.js line 6373 – 'code' is defined but never used. Allowed unused args must match /^.+/u. (no-unused-vars)

⚠️

iphone/cli/commands/_build.js#L6373 - iphone/cli/commands/build.js line 6373 – 'out' is defined but never used. Allowed unused args must match /^.+/u. (no-unused-vars)

Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3701 tests are passing.
(There are 470 tests skipped)

📖

🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

Generated by 🚫 dangerJS against 1c85a03

@janvennemann
Copy link
Contributor

janvennemann commented Jun 18, 2019

@cb1kenobi , i can still reproduce the issue with the following steps:

  1. Build and run any app using ti build -p ios
  2. Put the app into background and quit the ti build command
  3. Build and run the app again using ti build -p ios
  4. The build fails with the following error:
    [DEBUG] Checking if log server port 59584 is available
    [DEBUG] Log server port 59584 is in use, testing if it's the app we're building
    [ERROR] Another process is currently bound to port 59584
    [ERROR] Set a unique <log-server-port> between 1024 and 65535 in the <ios> section of the 
    tiapp.xml
    
  5. Put the app back into foreground, build and run again with ti build -p ios. This will now work again since the app is not in the background anymore.

@cb1kenobi
Copy link
Contributor Author

@janvennemann OK, I walked through things with @ewanharris and @vijaysingh-axway and came up with a totally rad solution. Give it a spin.

@cb1kenobi cb1kenobi self-assigned this Jun 19, 2019
@janvennemann
Copy link
Contributor

janvennemann commented Jun 24, 2019

@cb1kenobi hmm, if i understand correctly this will still fail the build, just because the app is backgrounded? This seems like an unnecessary inconvenience to me. As a user i don't want to manually bring back the app to foreground or kill it just so i can start a build.

Is there any chance we could improve this behavior on the simulator? Like alternating the ports between builds slightly so the running app in the background does not block the entire build? Or sending a proper quit message before exiting from our CLI that will make the app stop listening if no other connections are open?

@cb1kenobi
Copy link
Contributor Author

@janvennemann Just chatted with @ewanharris and talked about the tradeoffs that were made with the way things are today. I proposed that we revisit these tradeoffs in the next architecture meeting and come to a consensus how best to handle this going forward.

@cb1kenobi
Copy link
Contributor Author

@janvennemann OK, I've updated the code to pick a random port if the deterministic port is unavailable. Please give it a spin!

@hansemannn
Copy link
Collaborator

This works fine, we're using it in our fork already. Hope this can be merged on master soon!

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed Rebuilding the app with simulator no longer fails due to log port being occupied.

Tested with the following steps:

  1. Build and launch app on Simulator
  2. Put the app in the background
  3. Rebuild and see the error happening
  4. Bring the app to foreground, do another rebuild and everything works again

Test Environment

MacOS Mojave version 10.14.4
Xcode 10.2.1
Node.js ^8.12.0
iPhone 6s Sim(12.1).
"NPM":"4.2.14-3","CLI":"7.0.10"

@sgtcoolguy
Copy link
Contributor

I cloned this branch, fixed the ESLint issues, rebase down to single commit that matched our conventions and merged it to master.

@sgtcoolguy sgtcoolguy closed this Jul 9, 2019
@cb1kenobi cb1kenobi deleted the TIMOB-27074 branch July 9, 2019 15:30
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