Skip to content

Parse XCbuild stderr only if stdout is empty #3584

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

Merged
merged 4 commits into from
Jul 9, 2021
Merged

Parse XCbuild stderr only if stdout is empty #3584

merged 4 commits into from
Jul 9, 2021

Conversation

elsh
Copy link
Contributor

@elsh elsh commented Jul 1, 2021

SwiftPM parses all stderr output from XCBuild that shouldn't lead to a build failure, but emits it as an error diagnostic, which does lead to it. Diagnostics are already available via the parseable JSON output on stdout, not textually via stderr.

Motivation:

Resolves rdar://75751711 (SwiftPM is parsing ALL stderr output from XCBuild into an error diagnostic that fails the build).

Modifications:

This change parses XCBuild stderr and emits error only if stdout is empty.

Result:

If XCBuild stdout is not empty, stderr is now ignored, else it will throw a build error.

let redirection: Process.OutputRedirection = .stream(stdout: delegate.parse(bytes:), stderr: { bytes in
self.diagnostics.emit(StringError(String(bytes: bytes, encoding: .utf8)!))
if let str = String(bytes: bytes, encoding: .utf8) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably String(decoding: bytes, as: UTF8.self) is better here since it decodes as much content as possible even if there is invalid output.

let redirection: Process.OutputRedirection = .stream(stdout: delegate.parse(bytes:), stderr: { bytes in
self.diagnostics.emit(StringError(String(bytes: bytes, encoding: .utf8)!))
if let str = String(bytes: bytes, encoding: .utf8) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is it the right thing to emit all of this output as warnings? If I have an error that is emitted, would it show up as parseable diagnostics as well as on stderr, and thus show up as a warning as well as an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what the report in the radar seems to suggest. @jakepetroules Could you confirm the above question? Or would we need to filter and treat certain stderr as actual errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

All XCBuild output is communicated as structured (JSON) data via stdout, if we emit anything via stderr it's unintentional and can be dropped.

@elsh elsh requested a review from jakepetroules July 1, 2021 21:41
@elsh elsh changed the title Treat stderr from XCbuild as warning Parse XCbuild stderr only if stdout is empty Jul 7, 2021
@elsh
Copy link
Contributor Author

elsh commented Jul 7, 2021

@swift-ci smoke test

@tomerd
Copy link
Contributor

tomerd commented Jul 7, 2021

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented Jul 8, 2021

@swift-ci smoke test

Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

})

Copy link
Contributor

@tomerd tomerd Jul 9, 2021

Choose a reason for hiding this comment

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

remove empty line (format)

Suggested change

@elsh
Copy link
Contributor Author

elsh commented Jul 9, 2021

@swift-ci smoke test

@elsh elsh merged commit aa9ea35 into main Jul 9, 2021
@elsh elsh deleted the es-stderr branch July 9, 2021 21:59
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.

5 participants