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

Resolve xcodebuild output hangs #2297

Closed
wants to merge 16 commits into from
Closed

Resolve xcodebuild output hangs #2297

wants to merge 16 commits into from

Conversation

Jake-Prickett
Copy link
Contributor

@Jake-Prickett Jake-Prickett commented Jan 12, 2021

Resolves #2250
Request for comments document (if applies):

Short description πŸ“

Initial attempt at resolving the issue! πŸš€

Currently when calling tuist test or tuist build, during compilation the output hangs indefinitely when a warning or error is encountered. The reason being, the library xcbeautify leveraged to format the xcodebuild output was designed to have output piped into it (as opposed to calling the format methods explicitly from the library). With this design, the Library calls readLine() when an error or warning is encountered to consume and format a bit more information about the error/warning - because Tuist does not pipe the output, readLine() hangs indefinitely waiting for input from stdin.

Solution

Rather than format each line individually, leverage Foundation Process and Pipe to Pipe output from xcodebuild into xcbeautify.

Changes

  • Remove xcbeautify as a dependency from the project
  • Update the Rakefile to pull in the repository and build the binary version of xcbeautify
  • Add ability to find the xcbeautify binary to the BinaryLocator
  • Refactor the Parsing class to be a Formatter class that provides arguments for the xcbeautify command.
  • XcodeBuildController - when running a command, we now need to know if we should pipe the output through the formatter
    • Based on .isVerbose from the current environment
  • System - Created additional method to enable passing a second set of Arguments to pipe the output from the first Argument to.
    • Leverage Foundation.Process vs. swift-tools-support-core s Thread
  • Adjusted the XcodeBuildOutput to now only have a raw string, this is due to the fact that we no longer have access to both the formatted version and the raw version at the same time.
  • Updated some tests to ensure they stayed passing

TODO:

  • Finalize implementation based on feedback and make sure I followed standards :)
  • Add unit test coverage for the changes added

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • A developer other than the author has verified that the changes work as expected.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@pepicrft pepicrft requested review from a team, RomainBoulay and andreacipriani and removed request for a team January 13, 2021 17:23
@pepicrft
Copy link
Contributor

Hey @Jake-Prickett, thank you very much for contributing to the project. Few suggestions:

  • Rebase from main to fix the Next Website and Website failing workflows.
  • Run bundle exec rake style_correct to fix all the styling issues.
  • There seems to be some legit failing unit tests.

Moreover, I'd update the CHANGELOG to mention that we are fixing a known issue running tuist test and tuist test πŸš€

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Hey @Jake-Prickett. Thank you very much for contributing to the project.

@pepicrft
Copy link
Contributor

@all-contributors add @Jake-Prickett for code

@allcontributors
Copy link
Contributor

@pepibumur

I've put up a pull request to add @Jake-Prickett! πŸŽ‰

@Jake-Prickett Jake-Prickett marked this pull request as ready for review January 14, 2021 19:17
@Jake-Prickett
Copy link
Contributor Author

Thanks for the review @pepibumur and the tips! Up to date with the most recent main, resolved formatting issues, and fixed the test failure! πŸ‘

@fortmarek fortmarek self-requested a review January 14, 2021 21:47
Copy link
Contributor

@RomainBoulay RomainBoulay left a comment

Choose a reason for hiding this comment

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

Very solid work @Jake-Prickett πŸ‘ Thanks a lot for jumping on that πŸ™

Rakefile Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Good catch, found one small thing (maybe), thanks πŸ‘

Sources/TuistAutomation/Utilities/Formatter.swift Outdated Show resolved Hide resolved
@fortmarek
Copy link
Member

The unit tests seem to have failed, otherwise I think we are good to merge this

@Jake-Prickett
Copy link
Contributor Author

Jake-Prickett commented Jan 16, 2021

To be honest, I'm not entirely sure what the problem is. πŸ€”

It seems as though the 11.5 one is the issue if I'm not mistaken? Which led me to thinking it could be a difference with how xcbeautify may parse the output from xcodebuild for Xcode 11.5 (maybe a regex doesn't match or something?)

But when running locally (using both Xcode and command line) the tests that seem to fail/hang on CI pass without any issues on both Xcode 11.5 & Xcode 12.3.

Running the test that hangs:

Xcode 11.5 Command Line Tools

➜  tuist git:(resolve-xcodebuild-output-hangs) swift package clean
➜  tuist git:(resolve-xcodebuild-output-hangs) swift test --filter TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests
# ... compilation warnings, etc.
[1822/1822] Linking tuistPackageTests
Test Suite 'Selected tests' started at 2021-01-16 09:49:14.007
Test Suite 'tuistPackageTests.xctest' started at 2021-01-16 09:49:14.007
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' started at 2021-01-16 09:49:14.007
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_iOS_framework]' started.
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_iOS_framework]' passed (7.622 seconds).
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' passed at 2021-01-16 09:49:21.630.
	 Executed 1 test, with 0 failures (0 unexpected) in 7.622 (7.623) seconds
Test Suite 'tuistPackageTests.xctest' passed at 2021-01-16 09:49:21.630.
	 Executed 1 test, with 0 failures (0 unexpected) in 7.622 (7.623) seconds
Test Suite 'Selected tests' passed at 2021-01-16 09:49:21.630.
	 Executed 1 test, with 0 failures (0 unexpected) in 7.622 (7.624) seconds
Test Suite 'Selected tests' started at 2021-01-16 09:49:21.707
Test Suite 'tuistPackageTests.xctest' started at 2021-01-16 09:49:21.708
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' started at 2021-01-16 09:49:21.708
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_macOS_framework]' started.
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_macOS_framework]' passed (3.211 seconds).
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' passed at 2021-01-16 09:49:24.919.
	 Executed 1 test, with 0 failures (0 unexpected) in 3.211 (3.212) seconds
Test Suite 'tuistPackageTests.xctest' passed at 2021-01-16 09:49:24.919.
	 Executed 1 test, with 0 failures (0 unexpected) in 3.211 (3.212) seconds
Test Suite 'Selected tests' passed at 2021-01-16 09:49:24.919.
	 Executed 1 test, with 0 failures (0 unexpected) in 3.211 (3.212) seconds
Test Suite 'Selected tests' started at 2021-01-16 09:49:24.970
Test Suite 'tuistPackageTests.xctest' started at 2021-01-16 09:49:24.970
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' started at 2021-01-16 09:49:24.970
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_tvOS_framework]' started.
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_tvOS_framework]' passed (5.788 seconds).
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' passed at 2021-01-16 09:49:30.759.
	 Executed 1 test, with 0 failures (0 unexpected) in 5.788 (5.789) seconds
Test Suite 'tuistPackageTests.xctest' passed at 2021-01-16 09:49:30.759.
	 Executed 1 test, with 0 failures (0 unexpected) in 5.788 (5.789) seconds
Test Suite 'Selected tests' passed at 2021-01-16 09:49:30.759.
	 Executed 1 test, with 0 failures (0 unexpected) in 5.788 (5.790) seconds
Test Suite 'Selected tests' started at 2021-01-16 09:49:30.830
Test Suite 'tuistPackageTests.xctest' started at 2021-01-16 09:49:30.831
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' started at 2021-01-16 09:49:30.831
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_watchOS_framework]' started.
Test Case '-[TuistKitIntegrationTests.CacheXCFrameworkBuilderIntegrationTests test_build_when_watchOS_framework]' passed (6.366 seconds).
Test Suite 'CacheXCFrameworkBuilderIntegrationTests' passed at 2021-01-16 09:49:37.198.
	 Executed 1 test, with 0 failures (0 unexpected) in 6.366 (6.367) seconds
Test Suite 'tuistPackageTests.xctest' passed at 2021-01-16 09:49:37.198.
	 Executed 1 test, with 0 failures (0 unexpected) in 6.366 (6.367) seconds
Test Suite 'Selected tests' passed at 2021-01-16 09:49:37.198.
	 Executed 1 test, with 0 failures (0 unexpected) in 6.366 (6.368) seconds

Any suggestions on how to approach this one? It looks like the run was stuck for ~3 hours, I don't want to eat up CI resources for that long 🀯

@Jake-Prickett
Copy link
Contributor Author

Update:

I'm able to run the Unit Tests locally on both Xcode 11.5 and 12.3 without having any issues. Additionally, when running the Acceptance Tests (leveraging ./bin fourier test tuist acceptance ...) they pass as well. πŸ€”

It doesn't look to be related to xcbeautify from my most recent investigation. I feel like I'm missing something small, but hit a wall with debugging this one. I'm trying to avoid debugging on CI by pushing additional commits since it blocks the CI resources for quite some time. 🀯 Any suggestions or tips @pepibumur @RomainBoulay @fortmarek?

@pepicrft
Copy link
Contributor

I'm looking into this one

@pepicrft
Copy link
Contributor

@Jake-Prickett I moved your work to this PR so that I can debug and push a fix to the branch. I also invited you to the organization so that you can push to remote without having to fork the repo.

@pepicrft pepicrft closed this Jan 20, 2021
@Jake-Prickett
Copy link
Contributor Author

Thank you @pepibumur! πŸ™ I'll continue debugging on my end as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test run output hangs on application target
4 participants