Skip to content
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.

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

[5.10] Add logic to split command line arguments on Windows #1043

Merged

Conversation

z2oh
Copy link
Contributor

@z2oh z2oh commented Feb 1, 2024

  • Explanation: This change implemented Windows-specific command line argument splitting for compilation database fields; previously, the Windows path separator was being interpreted as an escape character.

  • Scope: Without this change, sourcekit-lsp does not work with compilation databases on Windows. This change refactors command line argument splitting logic into Unix and Windows variants (the Windows variant being new code), with the callsite selecting between them via #if os(Windows).

  • Issue: Windows path separators in compile_commands.json not parsed correctly #1020

  • Risk: Low; the change only affects Windows. As sourcekit-lsp was not working with compilation database on Windows before this change, there is no possible regression on this platform.

  • Testing: I have built and tested the 5.10 toolchain on Windows with this cherry-pick. CI will build and test for other platforms.

  • Reviewer: @ahoppen, who authored this change and @bnbarham on Add logic to split command line arguments on Windows #1028

@ahoppen
Copy link
Collaborator

ahoppen commented Feb 1, 2024

Could you reduce the change to not move splitShellEscapedCommand to a different file? Since we are fairly late in 5.10’s release cycle, this should be the minimal possible diff to fix the issue.

@z2oh z2oh force-pushed the jeremy/5.10/split-windows-command-line branch 2 times, most recently from 9ab9744 to 1e1d8d9 Compare February 1, 2024 18:46
@z2oh
Copy link
Contributor Author

z2oh commented Feb 1, 2024

Good point; I moved the parsing logic back into CompilationDatabase.swift and folded the tests into CompilationDatabaseTests.swift.

@ahoppen ahoppen force-pushed the jeremy/5.10/split-windows-command-line branch from 1e1d8d9 to c492c14 Compare February 1, 2024 21:01
Previously, we were splitting command line arguments on Windows using the same rules as on Unix, which was incorrect, most importantly because backslashes in the first component of a Windows command line invocation are not escaping anything but interpreted verbatim.

Fixes apple#1020
rdar://120809063
@ahoppen ahoppen force-pushed the jeremy/5.10/split-windows-command-line branch from c492c14 to 73266b5 Compare February 1, 2024 21:04
@ahoppen
Copy link
Collaborator

ahoppen commented Feb 1, 2024

Updated the PR to be purely additive in a way that is guaranteed to only affect Windows.

@ahoppen ahoppen requested a review from bnbarham February 1, 2024 21:07
@bnbarham
Copy link
Contributor

bnbarham commented Feb 1, 2024

@swift-ci please test

@ahoppen
Copy link
Collaborator

ahoppen commented Feb 1, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 7e2d80c into apple:release/5.10 Feb 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants