BuildSystem delegate API #3209

krzyzanowskim · 2021-01-20T20:29:40Z

BuildSystem progress notification API via delegate

Motivation:

The BuildOperation acts as a black box, that makes it impossible to get meaningful notifications about the build events.

forum thread: https://forums.swift.org/t/how-buildsystem-api-is-accessible-for-the-clients/43787

Modifications:

Introduce BuildSystemDelegate that can be used to track the progress of the build operation by the clients.

Result:

add optional delegate to SPMBuildCore.BuildSystem
implement delegate calls for BuildOperation and XcodeBuildSystem

abertelrud · 2021-01-20T21:03:21Z

This looks like a good addition to me! One question: what are the guarantees about when the delegate methods will be called, e.g. are they guaranteed to be called all on the same queue so that there won't be any overlap between them? I'm not that familiar with the concurrency behavior of the callbacks from llbuild; perhaps that's already part of the contract, but if not, would it make sense to have a serial queue that they're all invoked on so that ever client doesn't have to implement its own concurrency control?

krzyzanowskim · 2021-01-20T21:13:31Z

what are the guarantees about when the delegate methods will be called, e.g. are they guaranteed to be called all on the same queue so that there won't be any overlap between them?

the delegate queue is already there (frankly, I don't use it, but will by the time I'll finish PR).

abertelrud · 2021-01-20T21:33:34Z

what are the guarantees about when the delegate methods will be called, e.g. are they guaranteed to be called all on the same queue so that there won't be any overlap between them?

the delegate queue is already there (frankly, I don't use it, but will by the time I'll finish PR).

Great, thanks! Didn't see that there was a queue already; I was only looking at the new callsites. Thanks!

tomerd · 2021-01-28T20:38:06Z

@krzyzanowskim thanks for putting this together, do you feel it is ready for review? cc @abertelrud

tomerd · 2021-01-28T20:38:52Z

@swift-ci please smoke test

krzyzanowskim · 2021-01-28T20:58:11Z

@tomerd yes, please do.

abertelrud

This looks good to me; thank you for the additions! I had a couple of questions but they are minor.

abertelrud · 2021-02-01T20:58:57Z

Sources/Build/BuildOperation.swift

@@ -29,7 +32,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
    let packageGraphLoader: () throws -> PackageGraph

    /// The build delegate reference.


Since there are now two different delegates, it would be great to differentiate between their purpose in the comments. I think the one here is the one that performs required actions on behalf of the build system, whereas the new one is just advisory, IIUC.

naming is hard indeed, especially that "build system" is ambiguous in the context. There's llbuild build system, and there's SPM build system. Would that work: 7e4f0bd ?

Yes, that's clearer. Thanks!

abertelrud · 2021-02-01T21:04:09Z

It looks as if the failures are because the CMake scripts need to be updated (for the new files).

tomerd · 2021-02-01T22:13:32Z

@krzyzanowskim lets get CMake fixed up and add the relevant comments so we can merge this

tomerd · 2021-02-04T21:08:12Z

@swift-ci please test

tomerd · 2021-02-04T21:08:23Z

@swift-ci please smoke test

tomerd · 2021-02-04T21:08:50Z

@abertelrud please confirm changes look good based on you previous feedback so we can merge this

abertelrud · 2021-02-05T05:18:51Z

@abertelrud please confirm changes look good based on you previous feedback so we can merge this

Thanks @tomerd they look good to me. As far as I am concerned this can be merged.

abertelrud · 2021-02-05T05:18:59Z

@swift-ci please smoke test

tomerd · 2021-02-05T17:27:53Z

@swift-ci please smoke test

abertelrud · 2021-02-05T19:07:48Z

Looks like SourceKit-LSP is affected:

10:20:48 
/Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/sourcekit-lsp/Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift:47:29: error: 'BuildSystemDelegate' is ambiguous for type lookup in this context
10:20:48   public weak var delegate: BuildSystemDelegate? = nil
10:20:48                             ^~~~~~~~~~~~~~~~~~~
10:20:48 /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swiftpm/Sources/SPMBuildCore/BuildSystemDelegate.swift:14:17: note: found this candidate
10:20:48 public protocol BuildSystemDelegate: AnyObject {
10:20:48                 ^
10:20:48 /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/sourcekit-lsp/Sources/SKCore/BuildSystemDelegate.swift:17:17: note: found this candidate
10:20:48 public protocol BuildSystemDelegate: AnyObject {
10:20:48                 ^

abertelrud · 2021-02-05T19:08:33Z

Not sure if there is an interim change that can be put into SourceKit-LSP to avoid the ambiguity, or whether this PR needs to change. @benlangmuir what would you suggest?

benlangmuir · 2021-02-05T19:44:53Z

swiftlang/sourcekit-lsp#370

@swift-ci please smoke test

benlangmuir · 2021-02-05T19:46:07Z

Not sure if there is an interim change that can be put into SourceKit-LSP to avoid the ambiguity, or whether this PR needs to change. @benlangmuir what would you suggest?

I changed sourcekit-lsp (swiftlang/sourcekit-lsp#370) to use a fully qualified reference, which seemed to be sufficient.

krzyzanowskim · 2021-02-05T21:58:45Z

This got broken in swiftlang#3209 which was using `llbuildSwift` unconditionally. This is easy to miss, but in some environments we set `SWIFTPM_LLBUILD_FWK` and link against a pre-built framework which will only have the module `llbuild`. We should improve the testing here and build in both modes to avoid this kind of breakage in the future.

This got broken in #3209 which was using `llbuildSwift` unconditionally. This is easy to miss, but in some environments we set `SWIFTPM_LLBUILD_FWK` and link against a pre-built framework which will only have the module `llbuild`. We should improve the testing here and build in both modes to avoid this kind of breakage in the future.

krzyzanowskim added 12 commits January 12, 2021 20:43

Rename public BuildDelegate to internal BuildOperationDelegate

3104458

Make BuildOperationDelegate internal class

a09917b

Setup delegate links

034e382

Outline delegate protocol

39b9c55

Call BuildSystemDelegate methods

5f617b9

Add buildSystemDidCancel call

e1e9aa6

Report progress updates

331ee34

Set delegate at any point

5fbd4e8

Public default delegate implementation

fbc3a8d

Explicit module name

09c631a

Remove module name from function parameters

b099bed

Track progress update with raw messages

4d34b58

Call callback on delegate queue

f7ad71a

abertelrud requested review from neonichu, tomerd, abertelrud and aciidgh January 20, 2021 21:33

Implement BuildSystemDelegate for XCBuildSystem

b467e16

krzyzanowskim marked this pull request as ready for review January 20, 2021 22:04

krzyzanowskim requested a review from friedbunny as a code owner January 20, 2021 22:04

tomerd assigned abertelrud Jan 21, 2021

abertelrud approved these changes Feb 1, 2021

View reviewed changes

krzyzanowskim added 2 commits February 3, 2021 01:07

Clarify in the comment what build system is referred

7e4f0bd

Update CMake scripts with new files

475d1eb

neonichu approved these changes Feb 4, 2021

View reviewed changes

Disambiguate type

6901265

tomerd merged commit 6475635 into swiftlang:main Feb 6, 2021

neonichu mentioned this pull request Feb 9, 2021

Fix BuildOperationBuildSystemDelegateHandler #3249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BuildSystem delegate API #3209

BuildSystem delegate API #3209

krzyzanowskim commented Jan 20, 2021 •

edited

Loading

abertelrud commented Jan 20, 2021

krzyzanowskim commented Jan 20, 2021

abertelrud commented Jan 20, 2021

tomerd commented Jan 28, 2021 •

edited

Loading

tomerd commented Jan 28, 2021

krzyzanowskim commented Jan 28, 2021

abertelrud left a comment

abertelrud Feb 1, 2021

krzyzanowskim Feb 3, 2021

abertelrud Feb 5, 2021

abertelrud commented Feb 1, 2021

tomerd commented Feb 1, 2021

tomerd commented Feb 4, 2021

tomerd commented Feb 4, 2021

tomerd commented Feb 4, 2021

abertelrud commented Feb 5, 2021

abertelrud commented Feb 5, 2021

tomerd commented Feb 5, 2021

abertelrud commented Feb 5, 2021

abertelrud commented Feb 5, 2021

benlangmuir commented Feb 5, 2021

benlangmuir commented Feb 5, 2021

krzyzanowskim commented Feb 5, 2021

		@@ -29,7 +32,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
		let packageGraphLoader: () throws -> PackageGraph

		/// The build delegate reference.

BuildSystem delegate API #3209

BuildSystem delegate API #3209

Conversation

krzyzanowskim commented Jan 20, 2021 • edited Loading

Motivation:

Modifications:

Result:

abertelrud commented Jan 20, 2021

krzyzanowskim commented Jan 20, 2021

abertelrud commented Jan 20, 2021

tomerd commented Jan 28, 2021 • edited Loading

tomerd commented Jan 28, 2021

krzyzanowskim commented Jan 28, 2021

abertelrud left a comment

Choose a reason for hiding this comment

abertelrud Feb 1, 2021

Choose a reason for hiding this comment

krzyzanowskim Feb 3, 2021

Choose a reason for hiding this comment

abertelrud Feb 5, 2021

Choose a reason for hiding this comment

abertelrud commented Feb 1, 2021

tomerd commented Feb 1, 2021

tomerd commented Feb 4, 2021

tomerd commented Feb 4, 2021

tomerd commented Feb 4, 2021

abertelrud commented Feb 5, 2021

abertelrud commented Feb 5, 2021

tomerd commented Feb 5, 2021

abertelrud commented Feb 5, 2021

abertelrud commented Feb 5, 2021

benlangmuir commented Feb 5, 2021

benlangmuir commented Feb 5, 2021

krzyzanowskim commented Feb 5, 2021

krzyzanowskim commented Jan 20, 2021 •

edited

Loading

tomerd commented Jan 28, 2021 •

edited

Loading