SourceKitten icon indicating copy to clipboard operation
SourceKitten copied to clipboard

Windows Support

Open compnerd opened this issue 2 years ago • 5 comments

This initial patch series beings the work to support Windows. It is incomplete in the sense that there are multiple issues with the tests that need to be ironed out still. However, the core of the changes seem correct to partially work through the test suite.

Of note is that the implementation here significantly diverges from Linux in at least one one sense: it is generally far more approximate to Darwin as the Windows Swift toolchain is a complete distribution and that includes libclang. This toolchain is also (partially) able to host the ObjC paths though the Foundation headers are not available as that is not part of the toolchain distribution.

One of the biggest issues that currently need to be resolved in this core set of changes is the toolchain installation location.

If it is helpful/reasonable, it should be possible to peel off parts of this series into individual PRs to get them merged earlier rather than later to help get more things in place (and make it easier for others to contribute as well).

One final item of note: this is currently not possible to build for most. This is reliant on a toolchain build that is newer than anything available in a prebuilt snapshot. I hope that we can get a new snapshot built in the next few days.

compnerd avatar May 11 '23 17:05 compnerd

@jpsim I'm happy to break this down into separate PRs. I still do not have a good solution to the HACK commits, and would love suggestions from you. The latest snapshot 5/18 should be able to build this. There is one failure that I am seeing locally due to the need to re-generate the expected results.

compnerd avatar May 22 '23 15:05 compnerd

Hi @compnerd, first sorry for the delay (laid off, job search, other priorities, etc).

Second, it's fantastic to see SourceKitten working on Windows, massive kudos 🙌.

My main point of feedback is that some changes break the non-Windows build or functionality. For example, some of the hardcoded paths (e.g. C:\Program Files\Git\cmd\git.exe, .build\checkouts\, C:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr\bin\swift.exe) apply unconditionally on all OSes, where they should only be applied to Windows.

If you don't mind, I'll cherry pick some of your commits (keeping you as author) that don't break anything in the currently supported platforms to reduce the scope of the changes you want to land in this PR. As those smaller pieces get merged in, you can rebase this one as you fix the build/functionality regressions on macOS/Linux.

jpsim avatar Jun 13 '23 17:06 jpsim

Hi @compnerd, first sorry for the delay (laid off, job search, other priorities, etc).

Thank you for the response! Life often gets in the way, I hope that everything has worked out well now.

Second, it's fantastic to see SourceKitten working on Windows, massive kudos 🙌.

Thank you!

My main point of feedback is that some changes break the non-Windows build or functionality. For example, some of the hardcoded paths (e.g. C:\Program Files\Git\cmd\git.exe, .build\checkouts\, C:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr\bin\swift.exe) apply unconditionally on all OSes, where they should only be applied to Windows.

Absolutely on point. In fact, they are being broken even further as we speak :). They are not entirely portable, and I've tried to call them out in the commits with the "HACK" prefix. They are not ready at all for merging and actually need some guidance on how to best approach the problems there.

If you don't mind, I'll cherry pick some of your commits (keeping you as author) that don't break anything in the currently supported platforms to reduce the scope of the changes you want to land in this PR. As those smaller pieces get merged in, you can rebase this one as you fix the build/functionality regressions on macOS/Linux.

That sounds great! The changes are ordered such that the ones marked as HACK are situated on top to help splitting things that are ready from those that are not.

compnerd avatar Jun 13 '23 17:06 compnerd

I've cherry-picked and merged many of the commits from this PR in #779.

Could you please rebase this PR with the remaining changes and I can add feedback inline?

Or you can open new PRs for incremental changes as they're ready if you prefer, up to you.

jpsim avatar Jun 14 '23 16:06 jpsim

Rebased the changes. I think that we need to add a helper to locate the executable (basically implement a poor which). The location of swiftc.exe is going to change soon as well. This will break this and will need to be aware of looking up the paths by searching Path.

compnerd avatar Jun 14 '23 16:06 compnerd