Swap `BuildSystem` to remove build settings method #183

DavidGoldman · 2019-10-25T15:27:39Z

Instead the BuildSystem should inform its delegate of changes to build settings via fileBuildSettingsChanged

benlangmuir · 2019-10-25T16:30:55Z

@swift-ci please test

DavidGoldman · 2019-10-29T23:23:32Z

Friendly ping

Sources/SKCore/BuildServerBuildSystem.swift

Sources/SKCore/BuildSystem.swift

Sources/SKCore/BuildSystemList.swift

Sources/SKCore/FileBuildSettingsChange.swift

benlangmuir · 2019-11-05T05:24:59Z

Sources/SKCore/BuildServerBuildSystem.swift

  }
 }

 extension BuildServerBuildSystem: BuildSystem {

  /// Register the given file for build-system level change notifications, such as command
  /// line flag changes, dependency changes, etc.
-  public func registerForChangeNotifications(for url: LanguageServerProtocol.URL) {
+  public func registerForChangeNotifications(


The build server still has a settings request; I think we need to make that request here to get the initial settings.

Hmm I believe this is still necessary right?

@rmaz what do you think? should we change the protocol or manually fetch the settings here?

The register request should be enough to get the settings, the initial settings should be returned as soon as they are available. I don't believe we need the textDocument/sourceKitOptions request any more.

Okay, left this as is, we can remove the textDocument/sourceKitOptions in a follow up if this is working well

The way FileOptionsChangedNotification and registration is setup does not play nicely with files that are outside the build model. Right now, if a file is not known to the build server, it's not clear if it will fail to register (in which case it will not get a notification if it is later added), or if it will register but not provide initial "nil" settings (since FileOptionsChangedNotification does not have a way to represent lack of settings). For the same reason, a file that successfully registers will never have its settings removed.

In the previous model, we could handle this because the settings update would trigger re-querying the settings synchronously, which can fail.

If the server can tell that the file is outside, it should fail registration (and we handle that OK). Otherwise it should provide initial nil settings.

What will happen in these two cases?

file that starts unknown to build system is added to the build system (e.g. after you edit a config file). Do we ever get notified of the settings? I'm guessing not since registration failed.

file that is known by the build system is removed from the build system. How can we be notified that this happened when the update notification has no notion of nil/removed?

Ah, by If the server can tell that the file is outside, it should fail registration (and we handle that OK). Otherwise it should provide initial nil settings. I meant that the server should only fail registration if it knows that file can never be handled by the build system (e.g. for a monorepo this happens if it's in a different directory). Unless it can do that it should register and provide no settings.

re #2 hmm this we won't handle, @rmaz what do you think?

Sources/SourceKit/SourceKitServer.swift

DavidGoldman · 2020-05-20T19:18:10Z

As mentioned in https://forums.swift.org/t/sourcekit-lsp-file-status-ux/3594, planning on reviving this soon.

- Still need to update all of the `BuildSystem`s and tests Change-Id: I3d9cd921c650f782bbfa0628a85763cbf838e5cb

- With the `settings(for:)` method removed, we instead block on a callback from the `BuildSystemManager` which itself handles the primary build system callback in conjunction with the fallback system - We queue up notifications / requests that reference documents. TODO: - Full queueing for some text doc modifications, including save/edits. - Revisit test timeouts + modify BSM fallback timeout for tests Change-Id: I7cb24145cb87470a7b4701dc3b96c59ab18543c9

DavidGoldman · 2020-05-28T13:49:17Z

@swift-ci please test

DavidGoldman · 2020-05-28T13:49:36Z

@swift-ci please test

- Add more comments - willSave/didSave should be queued as well as changeDocument - Make the new BuildSystem requirements more clear - Remove the DispatchTimeInterval comparison check due to https://bugs.swift.org/browse/SR-6310, should not be needed anyway Change-Id: I6992b0cdba54ab6b4ccde304f8a2aecbba9e496a

DavidGoldman · 2020-05-28T15:11:54Z

@swift-ci please test

DavidGoldman · 2020-05-28T15:12:09Z

@swift-ci please test

Change-Id: I0436f9d854dfa85ff76e098e7c07b2c14c071d14

Sources/SKCore/BuildServerBuildSystem.swift

- Use the queue to update the language mapping - Handle register request failure Change-Id: I7fb19e518e134e3aeec2e4282d6298701ae48fce

DavidGoldman · 2020-05-28T20:03:18Z

@swift-ci please test

DavidGoldman · 2020-05-28T20:03:28Z

@swift-ci please test

benlangmuir

Reviewed the build system changes; still need to do a pass over the langauge server changes. Looking really good, thanks!

Sources/SKCore/BuildSystemManager.swift

Sources/SKCore/CompilationDatabaseBuildSystem.swift

- Fixes for BuildSystemManager and other BuildSystems - Remove language from FileBuildSettings as it is no longer needed, Swift-specific working dir fixes are now in SwiftCompileCommand Change-Id: Idff4371e6f6961c2c49998e9cb1dd0dd248da034

DavidGoldman · 2020-05-29T16:23:18Z

@swift-ci please test

DavidGoldman · 2020-05-29T16:23:22Z

@swift-ci please test

DavidGoldman · 2020-05-29T16:23:34Z

@swift-ci please test

Sources/SKCore/CompilationDatabaseBuildSystem.swift

Sources/SourceKit/SourceKitServer.swift

benlangmuir · 2020-05-29T17:01:07Z

Sources/SourceKit/SourceKitServer.swift

+    // No need to send the notification since it was never considered opened.
+    self.documentToPendingQueue[uri]?.cancelAll()
+    self.documentToPendingQueue[uri] = nil
+    self.documentsReady.insert(uri)


Why is it considered ready here?

oof nice catch, should instead be a removal above

This got marked as resolved by the code still has self.documentsReady.insert(uri). Did this change get missed/dropped?

You're looking at an older outdated version, or maybe github is confused? https://github.com/DavidGoldman/sourcekit-lsp-1/blob/469cfa49eac542f6161f1096c18824342344d20e/Sources/SourceKit/SourceKitServer.swift#L584

- registerForChangeNotifications should inform its delegate asynchronously instead of immediately - Use [weak self] for the pending requests/notifications to prevent retain cycle if we don't get a settings callback - Fix for never removing readyDocuments Change-Id: Id5c183bcc6a99f11b95bf8bfd8b9a93b3af39733

- Another `Language` param removal Change-Id: Ib1ebb7f2fee8f77a22c48438b80b197447041ae5

DavidGoldman · 2020-05-29T18:26:02Z

@swift-ci please test

DavidGoldman · 2020-05-29T18:26:07Z

@swift-ci please test

DavidGoldman · 2020-05-29T18:26:17Z

@swift-ci please test

DavidGoldman requested a review from benlangmuir as a code owner October 25, 2019 15:27

DavidGoldman mentioned this pull request Oct 25, 2019

Swap BuildSystem to remove build settings method #176

Closed

DavidGoldman force-pushed the public branch from 74df495 to 1ed418b Compare October 25, 2019 22:07

DavidGoldman commented Oct 29, 2019

View reviewed changes

Sources/SKCore/BuildServerBuildSystem.swift Outdated Show resolved Hide resolved

DavidGoldman force-pushed the public branch from 1ed418b to f206ab2 Compare October 30, 2019 19:04

benlangmuir reviewed Nov 5, 2019

View reviewed changes

Sources/SourceKit/SourceKitServer.swift Outdated Show resolved Hide resolved

rmaz mentioned this pull request Apr 2, 2020

Append working directory to swift compiler args when presented in build settings #260

Merged

Initial work to remove settings(for:) from the BuildSystem API

fca2571

- Still need to update all of the `BuildSystem`s and tests Change-Id: I3d9cd921c650f782bbfa0628a85763cbf838e5cb

DavidGoldman force-pushed the public branch from f206ab2 to fca2571 Compare May 21, 2020 20:46

Style: public extension members, not public extension

71f3e3e

Change-Id: I0436f9d854dfa85ff76e098e7c07b2c14c071d14

DavidGoldman commented May 28, 2020

View reviewed changes

Sources/SKCore/BuildServerBuildSystem.swift Show resolved Hide resolved

Minor fixes to the BuildServerBuildSystem

fe68859

- Use the queue to update the language mapping - Handle register request failure Change-Id: I7fb19e518e134e3aeec2e4282d6298701ae48fce

benlangmuir reviewed May 28, 2020

View reviewed changes

BuildSystem fixes

2e8b007

- Fixes for BuildSystemManager and other BuildSystems - Remove language from FileBuildSettings as it is no longer needed, Swift-specific working dir fixes are now in SwiftCompileCommand Change-Id: Idff4371e6f6961c2c49998e9cb1dd0dd248da034

benlangmuir reviewed May 29, 2020

View reviewed changes

DavidGoldman added 2 commits May 29, 2020 13:27

Improve handling of SourceKitServer's build system notifications

469cfa4

- Another `Language` param removal Change-Id: Ib1ebb7f2fee8f77a22c48438b80b197447041ae5

benlangmuir approved these changes Jun 1, 2020

View reviewed changes

DavidGoldman merged commit cf19900 into swiftlang:master Jun 1, 2020

Swap BuildSystem to remove build settings method #183

Swap BuildSystem to remove build settings method #183

Uh oh!

Conversation

DavidGoldman commented Oct 25, 2019

Uh oh!

benlangmuir commented Oct 25, 2019

Uh oh!

DavidGoldman commented Oct 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DavidGoldman commented May 20, 2020

Uh oh!

DavidGoldman commented May 28, 2020

Uh oh!

DavidGoldman commented May 28, 2020

Uh oh!

DavidGoldman commented May 28, 2020

Uh oh!

DavidGoldman commented May 28, 2020

Uh oh!

Uh oh!

DavidGoldman commented May 28, 2020

Uh oh!

DavidGoldman commented May 28, 2020

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidGoldman commented May 29, 2020

Uh oh!

DavidGoldman commented May 29, 2020

Uh oh!

DavidGoldman commented May 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidGoldman commented May 29, 2020

Uh oh!

Swap `BuildSystem` to remove build settings method #183

Swap `BuildSystem` to remove build settings method #183