Bug 179638 - webkitpy: Many error messages printed when running tests without an iOS SDK
Summary: webkitpy: Many error messages printed when running tests without an iOS SDK
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-13 15:10 PST by Jonathan Bedard
Modified: 2017-11-29 09:06 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.36 KB, patch)
2017-11-13 15:17 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-11-13 15:10:38 PST
We should suppress messages to stderr when checking on the installed SDK.  Checking style and running webkitpy tests on a Mac without an iOS SDK demonstrates the annoyance of this logging.
Comment 1 Radar WebKit Bug Importer 2017-11-13 15:10:57 PST
<rdar://problem/35516900>
Comment 2 Jonathan Bedard 2017-11-13 15:17:20 PST
Created attachment 326812 [details]
Patch
Comment 3 Alexey Proskuryakov 2017-11-13 18:52:00 PST
When did this start, is it new?
Comment 4 Jonathan Bedard 2017-11-13 19:06:42 PST
(In reply to Alexey Proskuryakov from comment #3)
> When did this start, is it new?

It's definitely not new.  test-webkitpy has probably had this problem since iOS Simulator testing was added.

I introduced this problem to check-webkit-style when I added checks for the TestExpectations.  I mention in <https://bugs.webkit.org/show_bug.cgi?id=179534> what I think the solution to the underlying problem with check-webkit-style should be.

I believe that the logging is an incidental consequence of how subprocess.popen works when passed stderr=None.  I don't think the intention was for stderr to be logged, especially when running unit tests.
Comment 5 Daniel Bates 2017-11-13 21:33:16 PST
Quickly glancing at this patch it just seems wrong. It would be helpful if you would post the error message that you are seeing. I could try to reproduce, but I am not at a computer with a checkout.
Comment 6 Daniel Bates 2017-11-13 21:36:47 PST
(In reply to Jonathan Bedard from comment #0)
> We should suppress messages to stderr when checking on the installed SDK. 
> Checking style and running webkitpy tests on a Mac without an iOS SDK
> demonstrates the annoyance of this logging.

This is an Apple Internal issue. It is not possible to obtain Xcode without an iOS SDK from developer.apple.com/App Store.
Comment 7 Jonathan Bedard 2017-11-14 08:44:46 PST
(In reply to Daniel Bates from comment #5)
> Quickly glancing at this patch it just seems wrong. It would be helpful if
> you would post the error message that you are seeing. I could try to
> reproduce, but I am not at a computer with a checkout.

It's definitely not helpful, for a few reasons:

1) Without this change, when we try and run a command without returning stderr, we subvert all of our output capturing, meaning tests will print to stderr even though we have code in place specifically disabling this

2) This function will be run multiple times because we don't have it memoized.  We SHOULDN'T have it memoized, because it's callers always cache the value in some form, but this means that the error message will print multiple times.

3) The comment indicates that we except to receive an empty string in some cases.  This is also the case that we will have stderr logging. Why should we log to stderr if the script is behaving as expected? If a caller of this function receives 'None' and expects a value, the exception will occur (and should be managed) there. It will be very obvious what the problem is.


The error printed is this:

xcodebuild: error: SDK "iphonesimulator" cannot be located.
scrum: error: unable to lookup item 'SDKVersion' in SDK 'iphonesimulator'
Comment 8 Jonathan Bedard 2017-11-15 09:21:21 PST
Comment on attachment 326812 [details]
Patch

I talked with Dan about this yesterday (11/14).

We're going to delay this change until work on <https://bugs.webkit.org/show_bug.cgi?id=179534> is completed, because at least some of the problems this patch is addressing will be fixed by those changes.
Comment 9 Jonathan Bedard 2017-11-29 09:06:57 PST
After applying the fix in <https://bugs.webkit.org/show_bug.cgi?id=179534>, this change is not needed.