Skip to content

Add support for different arguments per workspace #974

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 29, 2023
Merged

Add support for different arguments per workspace #974

merged 1 commit into from
Nov 29, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Nov 23, 2023

Fixes #663
rdar://101815704

@adam-fowler Do you think these new options on WorkspaceFolder would work for you? You should be able to specify them on both the initialize and the workspace/didChangeWorkspaceFolders request.

@ahoppen ahoppen requested a review from benlangmuir as a code owner November 23, 2023 02:17
@ahoppen
Copy link
Member Author

ahoppen commented Nov 23, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/build-settings-for-workspace-folder branch from a0a3bd0 to f4a7f57 Compare November 23, 2023 02:55
@adam-fowler
Copy link
Contributor

I think these should cover most things. It would be good to also include support for SDKs when support for these is added.

@ahoppen
Copy link
Member Author

ahoppen commented Nov 23, 2023

It would be good to also include support for SDKs when support for these is added.

I agree. Once we add that the SDK option to the command line options of sourcekit-lsp, we should also add it to the WorkspaceFolder options.

@ahoppen
Copy link
Member Author

ahoppen commented Nov 23, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Nov 24, 2023

@swift-ci Please test Windows

hamishknight
hamishknight approved these changes Nov 27, 2023
View reviewed changes
Sources/SourceKitLSP/SourceKitServer.swift Outdated
capabilityRegistry: capabilityRegistry,
toolchainRegistry: self.toolchainRegistry,
buildSetup: self.options.buildSetup,
buildSetup: workspaceBuildSetup.merging(self.options.buildSetup),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected that the main options can override the workspace options? I would have expected it to be the other way around

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, interesting. I thought of it as

  • Workspace options usually provide some reasonable defaults
  • The global options are the bigger hammer with which you can override everything, similar to how SWIFT_EXEC or TOOLCHAINS override any settings in your Xcode project.

But I’m not particularly to the precedence here. @adam-fowler Do you have any opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect the workspace options to override the global settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I’ll flip the precedence order.

@ahoppen ahoppen force-pushed the ahoppen/build-settings-for-workspace-folder branch 2 times, most recently from cbdc214 to 22ab201 Compare November 27, 2023 22:09
@ahoppen
Copy link
Member Author

ahoppen commented Nov 27, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Nov 27, 2023

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Nov 27, 2023

@swift-ci Please test Windows

@ahoppen ahoppen force-pushed the ahoppen/build-settings-for-workspace-folder branch from 22ab201 to 33f4612 Compare November 28, 2023 19:19
@ahoppen
Copy link
Member Author

ahoppen commented Nov 28, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Nov 28, 2023

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Nov 28, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 49d4db3 into swiftlang:main Nov 29, 2023
@ahoppen ahoppen deleted the ahoppen/build-settings-for-workspace-folder branch November 29, 2023 02:57
@ahoppen ahoppen mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adam-fowler adam-fowler adam-fowler left review comments

@hamishknight hamishknight hamishknight approved these changes

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@benlangmuir benlangmuir Awaiting requested review from benlangmuir

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for different arguments per workspace
3 participants
@ahoppen @adam-fowler @hamishknight