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

Add partial Windows support #779

Merged
merged 15 commits into from
Jun 14, 2023
Merged

Add partial Windows support #779

merged 15 commits into from
Jun 14, 2023

Conversation

jpsim
Copy link
Owner

@jpsim jpsim commented Jun 14, 2023

This cherry-picks the following commits from #769:

  • SourceKittenFramework: use free rather than UMRP.deallocate
  • SourceKittenFramework: treat Windows more similar to Darwin
  • SourceKittenFramework: add Windows spelling for sourcekitd
  • SourceKittenFramework: adjust import for Windows
  • SourceKittenFramework: support Windows SDK for Swift code
  • SourceKittenFramework: adjust source path computation
  • sourcekitten: import the Windows C library
  • Tests: disable syntaxtree tests on 5.9
  • Tests: add another version marker for Swift 5.9 (next?)
  • Tests: prefer XCTUnwrap over forced unwrapping
  • Tests: handle platform specific golden results
  • Tests: add the known set of strings for Windows

This isn't enough to get SourceKitten building and running on Windows, but rather are the building blocks that should be safe to merge as we work out the remaining issues.

compnerd and others added 13 commits June 14, 2023 10:05
The `UMRP.deallocate` will bridge to `_aligned_free` which requires that
the data be allocated with `_aligned_malloc`.  This does not hold as the
string is being duplicated via `strdup` and must be free'd explicitly
with `free`.  This happens to work on existing platforms as Darwin and
Linux do not use a separate memory arena for aligned memory allocations.
Windows packages libclang however the library name is spelled
differently. Adjust the library name for Windows support.
The Source Kit library on Windows is spelt `sourcekitdInProc.dll`.  Add
the alternate spelling to support Windows.
Add the Windows equivalent module name for the C library import that is
needed to build this module.
Windows uses the environment variable `SDKROOT` to specify a default SDK
to use for development.  The user may override this by passing
`-sdk ...` explicitly.  Use the environment variable for the tests.
Ensure that we convert to the real file path on the disk when we
serialise out the information.  Additionally, explicitly standardize the
path for Windows as it will fail to resolve drive substitutions
otherwise.
The Windows platform uses msvcrt which has been replaced by the UCRT and
is imported as `ucrt`.  Alter the import for the C library for the
`exit` function.
This was removed on 5.9 in favour of swift-syntax.  Disable the test on
5.9 or newer toolchain.
SourceKit introduced the new attribute `reusingastcontext` on a cursor
lookup.  Add the ability to accommodate that by suffixing another
version.
If there is a failure, the forced unwrapping will terminate the test
suite.  Prefer the `XCTUnwrap` to unwrap or fail instead.
Add a Windows specific expected results path.  This allows us to
exercise a greater number of tests on Windows than on Linux.
This is a terrible workaround to deal with the fact that `strings` is
not really available on Windows.  Without this it is not possible to
pass tests.
This keeps `SourceKitTests.swift` to a more reasonable length.
@compnerd
Copy link
Contributor

compnerd commented Jun 14, 2023

Would you be open to dropping the topmost two commits from the initially committed changes? I'd like to see if we can do something about that which avoids a special case for Windows. At least a discussion around how to best handle that and if we cannot find anything, we can do this.

@jpsim
Copy link
Owner Author

jpsim commented Jun 14, 2023

Would you be open to dropping the topmost two commits from the initially committed changes?

Sorry which two commits? These commits are the ones I think we can easily land, I just have a few test failures to address in 3 CI jobs, 5 of them are passing now.

Now that we are adding support for a 3rd platform, we should make the
library wrapper generation more robust.
@compnerd
Copy link
Contributor

The sourcekitStrings changes (057d71c and d1ba00f)

This would require that we have a separate hardcoded list for Windows and a dynamic list for non-Windows. I feel like that might make things confusing. I wonder if we can figure out how to get that set dynamically or statically everywhere.

@jpsim jpsim marked this pull request as ready for review June 14, 2023 16:47
@jpsim
Copy link
Owner Author

jpsim commented Jun 14, 2023

This would require that we have a separate hardcoded list for Windows and a dynamic list for non-Windows.

I'm ok with this for now. If you find a way to get this dynamically for Windows in the future, then we can replace this approach then, which I agree would certainly be nice.


I'll merge this now that CI is passing again with the currently supported platforms. Then you can rebase #769 with a more focused set of changes.

@jpsim jpsim merged commit 857250f into main Jun 14, 2023
@jpsim jpsim deleted the jp-basic-windows branch June 14, 2023 16:48
@jpsim jpsim mentioned this pull request Jun 14, 2023
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.

2 participants