Skip to content

Conversation

@neonichu
Copy link
Contributor

Right now we end up with this output on relink:

/Users/neonacho/Projects/public/swift-package-manager/.build/arm64-apple-macosx/debug/swift-bootstrap: replacing existing signature

This change specifically filters any output from codesign commands which haven't failed and we aren't in verbose mode.

This should eventually be possible at the command level, but for the time being this allows us to avoid shipping SwiftPM with this messy output.

rdar://118937939

@neonichu neonichu self-assigned this Jan 22, 2024
@neonichu
Copy link
Contributor Author

@swift-ci please test

@MaxDesiatov MaxDesiatov added the needs tests This change needs test coverage label Jan 22, 2024
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

As a stopgap measure this looks fine, but I think we should have at least two tests for potential regressions here:

  1. Verifying that diagnostic messages from other build commands (like driver and linker errors etc) come through
  2. Verifying that codesign output doesn't leak and the original issue is fixed

@neonichu
Copy link
Contributor Author

I think I can come up with a test for 2, but not sure what the concrete scenarios were for 1. I don't see any more info in the revert PR #7130.

@MaxDesiatov
Copy link
Contributor

Does the linked issue to the revert PR #7114 provide enough info? IIRC all error diagnostic messages were no longer visible between the original attempt to fix it and the revert

@neonichu
Copy link
Contributor Author

IIRC all error diagnostic messages were no longer visible between the original attempt to fix it and the revert

That doesn't seem plausible since we have tests involving diagnostics, e.g. various tests in CommandTests.BuildToolTests.

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Jan 22, 2024

Ok, some diagnostic messages didn't show up, as that's what @al45tair and @compnerd can confirm they were seeing as described in #7114 (comment), and I saw linker diagnostics swallowed too in my own builds.

@neonichu neonichu force-pushed the filter-replacing-existing-signature branch from 53d26bd to 8846f76 Compare January 23, 2024 21:59
@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu
Copy link
Contributor Author

BTW, the fact that compiler diagnostics were swallowed by the original change is very concerning to me -- this points to severe issues in SwiftPM's build system and/or the compiler/driver since compiler diagnostics are supposed to go through a completely separate path from this.

@neonichu neonichu force-pushed the filter-replacing-existing-signature branch from 8846f76 to 6b5b348 Compare January 24, 2024 01:34
@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu
Copy link
Contributor Author

@swift-ci please test windows

@neonichu neonichu removed the needs tests This change needs test coverage label Jan 24, 2024
@neonichu
Copy link
Contributor Author

ugh, testFilterNonFatalCodesignMessages also doesn't work on Linux because it re-uses the example...

Right now we end up with this output on relink:

```
/Users/neonacho/Projects/public/swift-package-manager/.build/arm64-apple-macosx/debug/swift-bootstrap: replacing existing signature
```

This change specifically filters any output from `codesign` commands which haven't failed and we aren't in verbose mode.

This should eventually be possible at the command level, but for the time being this allows us to avoid shipping SwiftPM with this messy output.

rdar://118937939
@neonichu neonichu force-pushed the filter-replacing-existing-signature branch from 6b5b348 to 09169fd Compare January 24, 2024 03:38
@neonichu
Copy link
Contributor Author

@swift-ci please test

@neonichu
Copy link
Contributor Author

@swift-ci please test windows

4 similar comments
@neonichu
Copy link
Contributor Author

@swift-ci please test windows

@neonichu
Copy link
Contributor Author

@swift-ci please test windows

@neonichu
Copy link
Contributor Author

@swift-ci please test windows

@neonichu
Copy link
Contributor Author

@swift-ci please test windows

@neonichu neonichu merged commit e930796 into main Jan 24, 2024
@neonichu neonichu deleted the filter-replacing-existing-signature branch January 24, 2024 17:41
@MaxDesiatov MaxDesiatov added bug build system Changes to interactions with build systems swift build Changes impacting `swift build` labels Jan 25, 2024
shahmishal pushed a commit that referenced this pull request Jan 26, 2024
* **Explanation**: 

SwiftPM is code signing any binaries it produces on macOS in 5.10.
Whenever a binary get relinked and resigned during incremental builds,
it verbatim emits informational diagnostics to the log, which should
only happen in verbose mode. Especially in projects which multiple
binaries, this can make the log difficult to follow and may lead to
confusion of users.

* **Scope**: Any users on macOS will see this issue.
* **Risk**: In theory, this change could lead to other diagnostics not
being emitted, but the change is scoped to only code signing tasks, so
the risk for this is very low.
* **Testing**: Some new tests for SwiftPM's logging were added as part
of this.
* **Reviewer**: @MaxDesiatov
* **Main branch PR**:
#7285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug build system Changes to interactions with build systems swift build Changes impacting `swift build`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants