Append working directory to swift compiler args when presented in build settings #260
Conversation
|
Currently this is only applied to the BuildServerBuildSystem, but it probably also should be done in at least the CompilationDatabaseBuildSystem as well. Maybe we should handle this in the SwiftLanguageServer code so that it applies to all sources of compiler arguments? @akyrtzi any concerns about that approach? |
|
I agree, the changes should be done in SwiftLanguageServer. I think longer term it would be ideal to remove the |
Seems like the right approach. |
|
@benlangmuir Thanks for the suggestion, now mutating args in SwiftLanguageServer itself |
| @@ -99,7 +99,7 @@ extension SwiftLanguageServer { | |||
|
|
|||
| // FIXME: SourceKit should probably cache this for us. | |||
| if let settings = self.buildSettingsByFile[uri] { | |||
| skreq[keys.compilerargs] = settings.compilerArguments | |||
| skreq[keys.compilerargs] = settings.getSwiftCompilerArguments() | |||
There was a problem hiding this comment.
Rather than do this every time, which is easy to forget, let's mutate the settings from the build system before we put them in self.buildSettingsByFile. I think there are two places (one synchronous call to settings and then one in the document update settings method.
There was a problem hiding this comment.
Mutating it would break the equity check against the build server setting in openDocument in swift language server
A nicer solution is to encapsulate buildSettingsByFile in swift language server and has its own equality check and setter, or, If we think this is the only way to sync the build server working directory setting, maybe we should change FileBuildSettings's init method directly?
| // but not in the actual compiler args. | ||
| extension FileBuildSettings { | ||
|
|
||
| func getSwiftCompilerArguments() -> [String] { |
There was a problem hiding this comment.
I suggest something like swiftCompilerArgumentsWithWorkingDirectory. Also, is this really a "FIXME" in the comment above? What would you do differently here? I would work that comment into the documentation comment for this API.
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| @testable import SKCore |
There was a problem hiding this comment.
We don't allow @testable imports, because it requires building in a way that's not compatible with how we want to distribute our binary. Feel free to make the API public to test it, but please (1) add an underscore, and (2) add "public for testing" at the front of its documentation comment.
|
Ah I missed that comment - what particularly would #183 help with here? It would mean we only need one place to accept arguments from the build system (the delegate method), but we'd still need to modify them to add the working directory, no? |
ghvg1313
force-pushed
the
swift-working-directory
branch
from
57f7f43 to
3a9f27b
Compare
|
@benlangmuir ping for review, changed logic to the file setting model now |
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors |
|
@swift-ci test |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test Linux |
|
|
@swift-ci test |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
|
@swift-ci please test |
|
@benlangmuir Safe to merge? |
Currently there's no explicit api for
sourcekitto recognize swift'sworking-directoryother than through compiler argsWhen build server contains such setting, inject the correct args in place.