Add support for building Swift on Windows with clang-cl and MSVC #5904

hughbe · 2016-11-22T18:04:52Z

Caveat

Please note: this PR on its own does not constitute actual support for building on Windows upon merging this PR.

For this, apple/swift-clang#45 and apple/swift-llvm#33 will have to be merged.
After this, I will break up my branch: https://github.com/hughbe/swift/tree/improve-win-instructions (that has a fully building Swift compiler) into multiple PRs (20 or so - 1 for each sub-project) and submit them.

If this is merged and you try to build Swift on Windows with MSVC, expect many thousands of build errors - many are duplicates, and all have been addressed in my branch above.

This could take a while, so hold your horses :)

Discussion

We get linker errors (unresolved external symbol _TMps16TextOutputStream) linking swiftPrivate.dll. For now building swiftPrivate is disabled in order to get the stdlib all building. This will need to be fixed at some point!

compnerd · 2016-11-27T22:40:07Z

cmake/modules/AddSwift.cmake

+        set(dep "${CMARK_LIBRARY_DIR}/cmark.lib")
+      else()
+        set(dep "${CMARK_LIBRARY_DIR}/lib${dep}.a")
+      endif()


This seems wrong. We already adjust the library prefix appropriately. Please adjust the conditions for that.

I'm not sure we adjust the prefix accordingly: the layout of the cmark build directory on Windows is as follows:

cmark.dll

cmark.exe

cmark.exp

cmark.ilk

cmark.lib

cmark.pdb

cmark_dll.pdb

cmark_export.h

cmark_static.lib

libcmark.pc

There is no such file "libcmark.a", so we do in fact need to adjust the depency for Windows.

Right, on windows, the library would be cmark.lib or libcmark.lib (in the vein of libcmt.lib). Sounds like I missed that when I first setup the windows support. We should fix that generically, not specifically for building with cl.

It is generic, and specific to Windows;

elseif("${dep}" STREQUAL "cmark") if ("${SWIFT_HOST_VARIANT_SDK}" STREQUAL "WINDOWS") set(dep "${CMARK_LIBRARY_DIR}/${dep}.lib") else() set(dep "${CMARK_LIBRARY_DIR}/lib${dep}.a") endif()

compnerd · 2016-11-27T22:40:42Z

cmake/modules/AddSwift.cmake

@@ -1085,7 +1092,7 @@ function(_add_swift_library_single target name)
        # libraries are only associated with shared libraries, so add an
        # additional check for that as well.
        set(import_library ${library})
-        if(TARGET ${library})
+        if(TARGET ${library} AND NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")


The import library path is needed on windows too. Why is this being changed?

Ah yeah I forgot to put this in the discussion point section in the PR description.
Basically, if we leave this code on Windows, I get the following error immediately when running the cmake build:

could not find dependency 'swiftCore_IMPLIB-NOTFOUND'
When this is commented out, everything seems to work.

Interesting. The problem is that when cross-compiling from Linux or macOS to Windows, you dont get the import library substitution behavior. As a result, we need to handle it manually. The import library property is manually set, so that should be present. It sounds like I may have gotten the condition wrong on that.

Okay, I assume that means we keep the system name check, or should I nerf this for cross compilation?

compnerd · 2016-11-27T22:41:03Z

cmake/modules/AddSwift.cmake

@@ -1833,7 +1840,21 @@ function(_add_swift_executable_single name)
      BINARY_DIR ${SWIFT_RUNTIME_OUTPUT_INTDIR}
      LIBRARY_DIR ${SWIFT_LIBRARY_OUTPUT_INTDIR})

+  # On Windows, libraries are prefixed with "lib", so we need to adjust FAT_LIBRARIES for this.


Windows doesnt support fat libraries.

Interesting - I'm fairly sure it just links as a library. Without this change, I get an error that goes something like cannot find library swiftReflection.lib. However, libSwiftReflection.lib does exist, and everything links correctly.

The output name should be swiftReflection.lib not libSwiftReflection.lib. Windows doesn't use the lib prefix on most libraries.

Odd: every single library built at the end of the swift build process has the prefix lib

That said, LLVM's built libraries don't have the lib prefix, so perhaps I just haven't ifdefed the place we add lib as a prefix to be non-Windows

I've removed the "lib" prefix from libraries, so this code can go as well.

compnerd · 2016-11-27T22:41:44Z

cmake/modules/AddSwift.cmake

+  # On Windows, we need to link rpcrt4.lib for UUIDs and mincore.lib for various file system APIs
+  if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
+    target_link_libraries("${name}" "mincore.lib" "rpcrt4.lib")
+  endif()


What libraries need this additional linkage? I added support for target specific linking. Ive never seen this need to be added. I think that this may be masking something else.

I'm linking rpcrt4.lib for creation of UUIDs that doesn't rely on libuuid which is tricky to build on Windows.
I'm linking mincore.lib in order to fix some unresolved symbol errors when linking Clang libraries, as Clang uses file system APIs found in mincore

The mincore linkage looks wrong, as that should be in the interface libraries for the clang library depending on it. The rpcrt4 linkage makes sense, but we should handle it as a replacement for uuid, and create a variable for uuid vs rpcrt4.

Right. Let me get back to you on what happens if i don't link mincore.lib (I'll run an entire build :) )

I get the following link errors:

clangDriver.lib(MSVCToolChain.cpp.obj) : error LNK2001: unresolved external symbol GetFileVersionInfoSizeW clangDriver.lib(MSVCToolChain.cpp.obj) : error LNK2001: unresolved external symbol GetFileVersionInfoW clangDriver.lib(MSVCToolChain.cpp.obj) : error LNK2001: unresolved external symbol VerQueryValueW bin\swift-ide-test.exe : fatal error LNK1120: 3 unresolved externals

compnerd · 2016-11-27T22:42:42Z

cmake/modules/AddSwift.cmake

-    ${${CFLAGS_RESULT_VAR_NAME}}
-    "-target" "${SWIFT_SDK_${CFLAGS_SDK}_ARCH_${CFLAGS_ARCH}_TRIPLE}")
+  # MSVC doesn't understand the option -target
+  if (NOT "${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")


Change this to:

if ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")

done, thanks

Wait, hang on, what about GCC?

I've fixed these checks to be not MSVC

compnerd · 2016-11-27T22:44:01Z

cmake/modules/AddSwift.cmake

-      list(APPEND result "--sysroot=${SWIFT_SDK_${CFLAGS_SDK}_PATH}")
-    endif()
+  elseif(NOT "${SWIFT_SDK_${CFLAGS_SDK}_PATH}" STREQUAL "/")
+    list(APPEND result "--sysroot=${SWIFT_SDK_${CFLAGS_SDK}_PATH}")
  endif()


Nice simplification, however, I think that you will need to complicate this with a ${CMAKE_C_COMPILER_ID} check.

Fixed, thanks

compnerd · 2016-11-27T22:44:21Z

cmake/modules/AddSwift.cmake

-  if("${CFLAGS_SDK}" STREQUAL "WINDOWS")
+  # MSVC doesn't understand -fms-compatibility-version. Also, these definitions should not be added here
+  # for MSVC, as the linker and compiler have different command line options
+  if("${CFLAGS_SDK}" STREQUAL "WINDOWS" AND NOT "${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")


Please check the compiler ID against Clang instead.

Fixed, thanks

compnerd · 2016-11-27T22:45:05Z

cmake/modules/AddSwift.cmake

+    list(APPEND result "-DLLVM_ON_WIN32")
+    list(APPEND result "-D_CRT_SECURE_NO_WARNINGS")
+    # TODO(compnerd) handle /MT
+    list(APPEND result "-D_DLL")


These are shared across MSVC and clang. This should be based on the target, not the compiler.

The issue here was that I get many warnings when linking using MSVC. Something like

no such option -DLLVM_ON_WIN32

no such option -D_CRT_SECURE_NO_WARNINGS

no such option -D_DLL

However, if we add these to the compiler flags instead of both the compiler and linker flags, we remove these warnings.

This should be based on the target

I don't fully understand this, sorrry

The spelling is /DLLVM_ON_WIN32 /D_CRT_SECURE_NO_WARNINGS /D_DLL with cl.

It doesn't matter if you use "-" or "/" in cl.exe. The problem that I'm mentioning is that the linker gets these flags as well as the compiler, when actually we only want the compiler to have these flags passed to it.

compnerd · 2016-11-27T22:46:50Z

cmake/modules/AddSwift.cmake

+      endif()
+    else()
+      list(APPEND result "-O0")
+    endif()


I think that splitting this out may make it more readable. We have similar behavior with different spellings. The difference is really the compiler id.

I haven't split this out, but I've tried to make this a bit more readable - let me know if you want me to split it out: I found it was too complicated and slightly confusing as you have to pass multiple variables etc. etc. Maybe that's because I'm not a CMake expert tho...

compnerd · 2016-11-27T22:49:49Z

cmake/modules/AddSwift.cmake

+
+    if(NOT "${SWIFT_${LFLAGS_SDK}_ICU_I18N}" STREQUAL "")
+      list(APPEND result "-L${SWIFT_${sdk}_ICU_I18N}")
+    endif()
  endif()


I would invert the conditions here, pushing the MSVC check inside the two clauses since the difference is the spelling.

Actually, this is slightly different, so can't be done.
The MSVC check is: if(NOT "${SWIFT_${LFLAGS_SDK}_ICU_UC_LIBRARIES}" STREQUAL "")
The Clang check is: if(NOT "${SWIFT_${LFLAGS_SDK}_ICU_UC}" STREQUAL "")

The variable should be the same, so something is wrong here.

You're right - I'm an idiot :D

compnerd · 2016-11-27T22:51:07Z

cmake/modules/AddSwift.cmake

+       ("${SWIFTLIB_SINGLE_SDK}" STREQUAL "WINDOWS" AND
+        NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "WINDOWS"))
+      list(APPEND link_flags "-fuse-ld=lld")
+    endif()


Hmm, I think that you may be breaking cross-compilation on Windows to Linux.

How would MSVC understand -fuse-ld=lld even if we're cross-compiling to Linux?

I've changed this code BTW to:

# MSVC doesn't recognise -fuse-ld. if ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang") if(SWIFT_ENABLE_GOLD_LINKER AND "${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_OBJECT_FORMAT}" STREQUAL "ELF") list(APPEND link_flags "-fuse-ld=gold") endif() if(SWIFT_ENABLE_LLD_LINKER OR ("${SWIFTLIB_SINGLE_SDK}" STREQUAL "WINDOWS" AND NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "WINDOWS")) list(APPEND link_flags "-fuse-ld=lld") endif() endif()

Perhaps this addresses the cross-compilation concerns?

Using clang on Windows to target Linux would be cross-compilation, which would use the -fuse-ld=gold option.

Yeah, so this now works, surely, as we have the if ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang") check?

compnerd · 2016-11-27T22:51:37Z

cmake/modules/AddSwift.cmake

+  else()
+    set_property(TARGET "${target}" APPEND_STRING PROPERTY
+      LINK_FLAGS " ${link_flags} -L${SWIFTLIB_DIR}/${SWIFTLIB_SINGLE_SUBDIR} -L${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFTLIB_SINGLE_SUBDIR} -L${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}")
+  endif()


Please combine the paths to use link_directories.

Done, cheers

compnerd · 2016-11-27T22:51:44Z

cmake/modules/AddSwift.cmake

+  else()
+    list(APPEND link_flags
+          "-L${SWIFTLIB_DIR}/${SWIFT_SDK_${SWIFTEXE_SINGLE_SDK}_LIB_SUBDIR}")
+  endif()


Done, cheers

compnerd · 2016-11-27T22:51:57Z

cmake/modules/AddSwift.cmake

+       ("${SWIFTLIB_SINGLE_SDK}" STREQUAL "WINDOWS" AND
+        NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "WINDOWS"))
+      list(APPEND link_flags "-fuse-ld=lld")
+    endif()


Again, cross-compiling from Windows to Linux.

See the comment 2 above:
How would MSVC understand -fuse-ld=lld even if we're cross-compiling to Linux?

I've changed this code BTW to:

# MSVC doesn't recognise -fuse-ld. if ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang") if(SWIFT_ENABLE_GOLD_LINKER AND "${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_OBJECT_FORMAT}" STREQUAL "ELF") list(APPEND link_flags "-fuse-ld=gold") endif() if(SWIFT_ENABLE_LLD_LINKER OR ("${SWIFTLIB_SINGLE_SDK}" STREQUAL "WINDOWS" AND NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "WINDOWS")) list(APPEND link_flags "-fuse-ld=lld") endif() endif()

Perhaps this addresses the cross-compilation concerns?

compnerd · 2016-11-27T22:52:49Z

cmake/modules/SwiftSharedCMakeConfig.cmake

+  endif()
+
+  # MSVC can't include directories at this point - we need to wait until _add_swift_library_single
+  if (NOT "${CMAKE_C_COMPILER_ID}" STREQUAL "Windows")


ITYM MSVC.

Keen eyes, good spot. Doesn't affect anything, as cmake just ignores the call, but thanks :)

compnerd · 2016-11-27T22:53:25Z

CMakeLists.txt

@@ -467,6 +469,12 @@ else()
    set(SWIFT_HOST_VARIANT_ARCH_default "armv6")
  elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "armv7l")
    set(SWIFT_HOST_VARIANT_ARCH_default "armv7")
+  elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "AMD64")
+    set(SWIFT_HOST_VARIANT_ARCH_default "x86_64")
+  elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "IA64")


IA64 is itanium, not x86_64.

Cheers, fixed

compnerd · 2016-11-27T22:54:10Z

CMakeLists.txt

  set(SWIFT_PRIMARY_VARIANT_ARCH_default "x86_64")

+elseif("${SWIFT_HOST_VARIANT_SDK}" STREQUAL "WINDOWS")
+  configure_sdk_windows(WINDOWS "Windows" "pc" "msvc" "${SWIFT_HOST_VARIANT_ARCH}")


Why use the pc vendor? unknown should be fine.

Right. Once we get to the awesome point of compiling the stdlib and the compiler, I ran swiftc helloworld.swift, and got an error that goes something like this:
could not find stdlib x86_64-pc-windows-msvc

updating unknown to pc fixes this

That would be because we didnt setup the resource dir correctly. Fixing that would address this I believe.

What do you mean by resource dir? Thanks!

The resource dir is a special directory that has compiler resources (headers, libraries). It is controlled via the -resource-dir parameter which IIRC we explicitly pass to the compiler in the build system

compnerd · 2016-11-27T22:55:06Z

CMakeLists.txt

+  elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "IA64")
+    set(SWIFT_HOST_VARIANT_ARCH_default "x86_64")
+  elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "x86")
+    set(SWIFT_HOST_VARIANT_ARCH_default "x86")


Can we not rewrite this as i686 internally rather than propagating x86?

Sure - I was unfamiliar with this kinda arch stuff, so it's great to have another set of eyes on this!
I haven't tested on x86 as I don't have an x86 PC!

Heh. Apple platforms prefer (require?) i386. Not sure what the right thing to do here is.

compnerd

We should split this out and get it merged first.

compnerd · 2016-11-27T22:55:29Z

.gitignore

+#==============================================================================#
+8
+4
+SortedCFDatabase.def


Missing newline.

compnerd · 2016-11-27T22:56:00Z

cmake/modules/SwiftHandleGybSources.cmake

+  else()
+    set(gyb_tool_command ${gyb_tool})
+  endif()
+


You can always run it this way irrespective of the platform. I think having a single way to run it is better.

Fixed, thanks

compnerd · 2016-11-27T22:57:07Z

cmake/modules/SwiftHandleGybSources.cmake

+      # On Windows (using Visual Studio), the generated project files assume that the
+      # generated GYB files will be in the source, not binary directory.
+      # TODO: when we use Ninja as a generator, this may be unecessary.
+      IF (WIN32)


Lowercase if, no space after it. Doesnt this mean that you are no longer doing an out-of-source compilation?

Yes, and I've just fixed this for Ninja. For some reason Visual Studio's generated project assume the generated GYB files are in the source directory. Ninja doesn't care, so I've nerfed VS and left the old out of source compilation for Ninja.

compnerd · 2016-11-27T23:01:13Z

tools/SourceKit/tools/CMakeLists.txt

+# Also, see FIXME in Repl.cpp:
+# FIXME: Support REPL on non-Apple platforms. Ubuntu 14.10's editline does not
+# include the wide character entry points needed by the REPL yet.
+if(NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")


This should be handled similar to libxml2 with a SWIFT_HAVE_LIBEDIT.

Good idea, thanks

compnerd · 2016-11-27T23:01:28Z

tools/driver/CMakeLists.txt

 # CMake doesn't support create_symlink on Windows: http://cmake.3232098.n2.nabble.com/cmake-E-create-symlink-for-Windows-td4151271.html
+# TODO(hughbe/Windows): Windows does not support libedit currently. See the TODO in tools/SourceKit/tools/CMakeLists.txt


I don't think that this comment adds anything.

You're right - removed

compnerd · 2016-11-27T23:02:41Z

tools/swift-ide-test/CMakeLists.txt

@@ -12,7 +12,9 @@ add_swift_host_tool(swift-ide-test
 )

 # If libxml2 is available, make it available for swift-ide-test.
-if(SWIFT_HAVE_LIBXML)


I think that setting SWIFT_HAVE_LIBXML should work for this guard. Consider cross-compilation.

Fixed, thanks

compnerd

This is because clang is required: you need modules. There are module definitions that need to be pulled into the system SDK.

hughbe · 2016-11-28T10:44:11Z

This is because clang is required: you need modules. There are module definitions that need to be pulled into the system SDK.

How do we overcome this?

hughbe · 2016-11-28T10:45:49Z

I'm gonna also tag some Apple folks so they can chip in (no excuses now that Thanksgiving is over ;) )
@gottesmm @gribozavr @jrose-apple
Also, @modocache for your previous experience portin'

modocache · 2016-11-28T14:57:16Z

cmake/modules/SwiftHandleGybSources.cmake

@@ -42,6 +42,10 @@ function(handle_gyb_source_single dependency_out_var_name)
      ${GYB_SINGLE_FLAGS})

  set(gyb_tool "${SWIFT_SOURCE_DIR}/utils/gyb")
+
+  # GYB needs to be run via Python
+  set(gyb_tool_command "python" ${gyb_tool})


Should this be ${PYTHON_EXECUTABLE}? The executable name could be something other than "python", I think.

Interesting suggestion, I didn't know about this. Quick question: how does this interact with multiple python versions?
I found that you can't actually compile several gyb files if you use python 3 as they rely on python 2 code (e.g. maketrans/capitalize)

@hughbe that works by allowing you to specify the path to the python interpreter at the cmake configuration time (i.e. -DPYTHON_EXECUTABLE=/usr/x86_64-pc-linux-gnu/bin/python2.7).

done, thanks folks

modocache · 2016-11-28T14:57:21Z

cmake/modules/SwiftHandleGybSources.cmake

@@ -42,6 +42,10 @@ function(handle_gyb_source_single dependency_out_var_name)
      ${GYB_SINGLE_FLAGS})

  set(gyb_tool "${SWIFT_SOURCE_DIR}/utils/gyb")
+
+  # GYB needs to be run via Python
+  set(gyb_tool_command "python" ${gyb_tool})


Should this be ${PYTHON_EXECUTABLE}? The executable name could be something other than "python", I think.

done, thanks

modocache · 2016-11-28T15:05:56Z

stdlib/public/runtime/CMakeLists.txt

@@ -84,7 +84,7 @@ add_swift_library(swiftRuntime OBJECT_LIBRARY TARGET_LIBRARY
  C_COMPILE_FLAGS ${swift_runtime_library_compile_flags}
  LINK_FLAGS ${swift_runtime_linker_flags}
  INSTALL_IN_COMPONENT never_install
-  TARGET_SDKS ANDROID CYGWIN FREEBSD LINUX)
+  TARGET_SDKS ANDROID CYGWIN FREEBSD LINUX WINDOWS)


As part of a separate PR, I think a cleaner way to do this would be to define lists named OBJC_INTEROP_PLATFORMS and NO_OBJC_INTEROP_PLATFORMS, and use them instead of ALL_APPLE_PLATFORMS.

Here's a zany idea: to support projects like mulle-objc, we allow the OBJC_INTEROP_PLATFORMS and NO_OBJC_INTEROP_PLATFORMS lists to be configurable via the build script and CMake. It would be really cool if I could compile for macOS without Objective-C support enabled, or compile for Linux with Objective-C interop:

$ utils/build-script --target-no-objc-sdks macos $ utils/build-script --target-objc-sdks linux,freebsd

If this sounds like something the core team would be interested in, I'd love to submit a PR for it! /cc @gottesmm

Take a look at: #5942

Thanks! I also sent an email to the swift-dev mailing list.

@modocache mulle-objc is unlikely to work with the objc-interop. There are hardcoded assumptions in the compiler about the ObjC ABI that is being used. It MUST be the non-fragile iOS ABI that is used.

Let's discuss on-list to keep this pull request focused on just Windows. :) Even if mulle-objc doesn't work 100%, I think it's a good idea to allow Objective-C interop to be turned on or off for any given target.

compnerd · 2016-11-28T16:19:33Z

@hughbe as to how to overcome the modules issue: simple, dont use MSVC until such a day as it actually supports modules. You already must build clang to build swift. So you already have clang. You can use the just built clang to build the runtime.

That said, it seems that if you are sufficiently clever, you should be able to just copy the module maps into the appropriate SDK locations, and then use cl to build swiftc and the runtime and then use swift to build just the stdlib, which would use clang, which would allow you to use modules. Although, since the swift compilation is done manually .... this may actually be easier in practice than it appears at first glance.

jckarter · 2016-11-28T20:55:50Z

The runtime will eventually require that it be built with Clang once we enable the Swift calling convention too.

hughbe · 2016-11-29T16:00:24Z

The runtime will eventually require that it be built with Clang once we enable the Swift calling convention too.

I had a feeling that would happen - basically this gets us building with MSVC, but that's not really the complete goal. Once this is merged, I'll add support for clang-cl which should get the runtime working in the long run for a long time. This would be a rather simple task, as it should be as easy as adding CMAKE_CXX_SIMULATE_ID checks for clang-cl as this lays the groundwork.
That said, I'm probably gonna encounter difficulty, as is the way with these things.

jrose-apple · 2016-11-29T17:26:36Z

cmake/modules/AddSwift.cmake

@@ -820,6 +820,9 @@ function(_add_swift_library_single target name)
      if(NOT "${SWIFT_${SWIFTLIB_SINGLE_SDK}_ICU_I18N_INCLUDE}" STREQUAL "${SWIFT_${SWIFTLIB_SINGLE_SDK}_ICU_UC_INCLUDE}")
        include_directories("${SWIFT_${SWIFTLIB_SINGLE_SDK}_ICU_I18N_INCLUDE}")
      endif()
+
+      # We need to include cmark at this point (not in swift_common_standalone_build_config_cmark) using MSVC


My understanding is that, at the point at which CMark was included before this change, there was no target that we were adding these flags. For other compilers, this is fine. However, CMake's MSVC generator understands this as a nop and doesn't do anything.

jrose-apple · 2016-11-29T17:30:25Z

stdlib/public/core/CMakeLists.txt

-  # the second, it causes errors when the system libraries are told to
-  # include everything). The best way to get it in there, according to the
-  # documentation, is to put the flags in the target_link_libraries setting.
+    # With the GNU linker the equivalent of -all_load is to tell the linker


Why the extra indentation here?

No clue, reverted.

jrose-apple · 2016-11-29T17:30:44Z

stdlib/public/SwiftShims/CMakeLists.txt

@@ -79,13 +79,20 @@ else() # NOT SWIFT_BUILT_STANDALONE
  set(clang_headers_location "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION}")
 endif()

+# CMake doesn't support create_symlink on Windows: http://cmake.3232098.n2.nabble.com/cmake-E-create-symlink-for-Windows-td4151271.html
+if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
+set(symlink_command "mklink")


Please indent.

jrose-apple · 2016-11-29T17:31:34Z

tools/driver/CMakeLists.txt

@@ -13,18 +13,38 @@ add_swift_host_tool(swift
 )

 target_link_libraries(swift edit)
+# CMake doesn't support create_symlink on Windows: http://cmake.3232098.n2.nabble.com/cmake-E-create-symlink-for-Windows-td4151271.html


Should we factor this out into a helper in cmake/modules/?

Can we wait on this? The reason why I ask is that I'm going to submit a PR fixing some symlink issues on Windows, and this code will go.
The problem is that there is no good way to check if a symlink exists on Windows using CMake, and mklink reports an error which breaks/stops the build.
The fix would be to create a windows specific (or Xplat python script) that checks if a symlink exists before creating it.

jrose-apple · 2016-11-29T17:32:47Z

cmake/modules/SwiftSource.cmake

+  # Python utility script (line-directive-file), which reads the list of file paths,
+  # passing them to the old Python utility script (line-directive)
+  # TODO: we should move non-Windows builds to use this code at some point to
+  # remove code duplication


I think we should just do that now, before landing the patch.

Done, thanks

jrose-apple · 2016-11-29T17:34:37Z

cmake/modules/AddSwift.cmake

@@ -294,8 +294,12 @@ function(_add_variant_swift_compile_flags
    sdk arch build_type enable_assertions result_var_name)
  set(result ${${result_var_name}})

+  # On Windows, we don't set SWIFT_SDK_WINDOWS_PATH, so don't include it


The compiler behaves differently when there's an SDK provided and when there isn't. Is there nothing reasonable to default this to?

Actually, never mind. Most of those differences have been removed.

This is actually a dead comment, I have removed it

EDIT: I'm wrong

llvm-beanz · 2016-11-30T17:06:30Z

CMakeLists.txt

+
+  # We don't have a build script for Windows currently (build-script-impl is a bash script), so temporarily shortens and simplifies the build
+  # command for Swift on Windows - we only need to define 1 CMAKE variable (-DSWIFT_WINDOWS_ICU), rather than 8 (that are all the same anyway!)
+  # We also want to avoid a dependency on PkgConfig (find_package), which is not easily available on Windows.


Uh... This is not the right way to handle this. find_package doesn't require PkgConfig. The more correct solution would be to implement a "FindICU.cmake" module and have that module support overriding the ICU directory in a more universal way (it doesn't need to be Windows-specific).

Right - we have a FindICU.cmake module already, but we can see about fixing that file

jrose-apple · 2016-11-30T17:39:57Z

cmake/modules/AddSwift.cmake

-    ${${CFLAGS_RESULT_VAR_NAME}}
-    "-target" "${SWIFT_SDK_${CFLAGS_SDK}_ARCH_${CFLAGS_ARCH}_TRIPLE}")
+  # MSVC doesn't understand the option -target
+  if ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")


Again, what about GCC?

I didn't see that part of your review, but have addressed it and the other parts just now - sorry!

jrose-apple · 2016-11-30T17:40:44Z

cmake/modules/AddSwift.cmake

-    if("${CFLAGS_ARCH}" STREQUAL "i386" OR "${CFLAGS_ARCH}" STREQUAL "x86_64")
-      list(APPEND result "-momit-leaf-frame-pointer")
+    if("${CFLAGS_ARCH}" STREQUAL "i386" OR "${CFLAGS_ARCH}" STREQUAL "i686" OR "${CFLAGS_ARCH}" STREQUAL "x86_64")
+      if ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")


Yeah, this definitely needs to be a test for MSVC. More compilers are GCC-like than MSVC-like.

Make sense, fixed

hughbe · 2016-12-01T15:29:33Z

Okay, thanks all for your comments. I've addressed all of them hopefully. I've had to rebase due to merge conflicts. I also ran a Clang build on my Linux VM and that seems to be working (had to fix the new line-directive-file-script for python using bash).
I also simplified some of the ICU stuff and addressed some TODOs

jrose-apple · 2016-12-01T16:54:25Z

CMakeLists.txt

@@ -148,6 +148,10 @@ set(SWIFT_TOOLS_ENABLE_LTO OFF CACHE STRING "Build Swift tools with LTO. One
 set(SWIFT_PARALLEL_LINK_JOBS "" CACHE STRING
  "Define the maximum number of linker jobs for swift.")

+option(SWIFT_HAVE_LIBEDIT
+    "Enable linking to BSD's libedit. This is necessary to support the REPL and SourceKit, but is optional."


Nitpick: let's change "the REPL and SourceKit" to "the testing REPLs for swiftc and SourceKit". The real Swift REPL is in LLDB, and the rest of SourceKit should work fine without sourcekitd-test.

Fixed, thanks Jordan

jrose-apple · 2016-12-01T16:58:57Z

cmake/modules/AddSwift.cmake

@@ -356,10 +393,11 @@ function(_add_variant_link_flags)
  endif()

  if(NOT "${SWIFT_${LFLAGS_SDK}_ICU_UC}" STREQUAL "")
-    list(APPEND result "-L${SWIFT_${sdk}_ICU_UC}")
+    link_directories("${SWIFT_${sdk}_ICU_UC}")


If I'm reading the docs right, this adds these link directories to every target in the file, not just the one currently being edited (and same for include_directories below). While that's probably not harmful, it's not correct to do within _add_variant_link_flags either. Either we should set this all up initially, or we should leave it the way it is using manual flags.

I had no idea, but I think you're right.

and same for include_directories below

I think target_include_directories addresses this concern
I'm checking out the equivilent for link_directories

Unfortunately, we have to resort to an CMAKE_C_COMPILER_ID check for linking directories and do it manually with -L or /LIBPATH

Yeah, I see why you did it. Oh well. :-/

I actually think these might just be dead code, as we parse an absolute path to target_link_libraries or whatever it is, so these flags may be unecessary, but that's for another cleanup PR

jrose-apple · 2016-12-01T17:03:35Z

tools/swift-remoteast-test/CMakeLists.txt

-target_link_libraries(swift-remoteast-test edit)
+
+# TODO(hughbe/Windows): Windows does not support libedit currently. See the TODO in tools/SourceKit/tools/CMakeLists.txt
+if(NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")


SWIFT_HAVE_LIBEDIT?

missed that, fixed

jrose-apple · 2016-12-01T17:04:11Z

utils/line-directive

@@ -244,8 +245,15 @@ def run():
        dashes = sys.argv.index('--')
        sources = sys.argv[1:dashes]

+        # subprocess.Popen doesn't normalise paths on Windows (e.g. C:/swift/./lib)
+        # and instead reports file/directory not found errors. Therefore, we need
+        # to manually nromalise the path to the script to invoke. The rest will be


Typo: nromalise.

jrose-apple · 2016-12-02T16:57:50Z

That's all of my comments, but @compnerd and @llvm-beanz might still have more. (And yes, in the long-term we definitely should be building the runtime with the just-built Clang.)

llvm-beanz · 2016-12-02T22:40:24Z

I still think you should change how you're handling finding ICU and move the code into the FindICU.cmake file so that you're within CMake's convention.

hughbe · 2016-12-02T23:28:43Z

I still think you should change how you're handling finding ICU and move the code into the FindICU.cmake file so that you're within CMake's convention.
👍 2

You're right - I don't really know where I was coming from in my previous attempt!! So ugly, unconventional and unecessary.
I've fixed this and rebased to make things simple. Only thing that's changed is this commit: dfea553

Its probably most easily reviewed with ?w=1 flag, as that ignores whitepace changes (due to new if statement for Windows, to avoid PkgConfig dependency)

llvm-beanz · 2016-12-02T23:35:57Z

I've fixed this and rebased to make things simple. Only thing that's changed is this commit: dfea553

That commit doesn't seem to be part of the PR, and when I click the link for it nothing shows up.

hughbe · 2016-12-02T23:37:09Z

My bad, I fixed some comment typos so had to rebase again: 1a8d539

llvm-beanz · 2016-12-02T23:47:00Z

cmake/modules/FindICU.cmake

-      HINTS ${PC_ICU_${MODULE}_LIBDIR} ${PC_ICU_${MODULE}_LIBRARY_DIRS})
-    set(ICU_${MODULE}_LIBRARIES ${ICU_${MODULE}_LIBRARY})
+
+if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")


I think you're still not approaching this correctly.

Instead of having the if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows"), why not allow the ICU paths to be overridden on all platforms? Then the PkgConfig code path can run whenever an override value isn't specified on any platform, and raise errors if it fails.

It is worth noting that the PkgConfig functions in CMake return not-found if the user doesn't have pkg-config installed, so you don't need to avoid calling those functions on Windows (or any other platform).

Yeah you're right. The reason why I was avoiding all of this way because you now have to pass 6 command line variables to CMake, whereas before you only had to pass 1. I guess this is fine, because when we have a build-script it will be simpler.

Take a look at the Simplify ICU package resolution... commit: its a lot smaller, and only invokes certain behaviour if you can't find PkgConfig. I had to add two CMake variables (ICU_UC_LIB_NAME and ICU_I18N_LIB_NAME), because the library names for ICU on Windows and non-Windows are different. This way its user-configurable though.

I've updated the documentation accordingly.

Thanks

evieluvsrainbows

LGTM 👍

hughbe · 2016-12-22T11:18:50Z

Okay - I've applied some knife cutting to this PR, and have changed several commits and extracted them to their own PRs. This PR is now limited to clang-cl and MSVC specific functionality. E.g. compiler arguments, and linker errors.

out of date, please re-review :)

hughbe · 2017-02-20T10:08:12Z

@swift-ci please clean smoke test

hughbe · 2017-02-20T10:08:34Z

@swift-ci please clean smoke test

hughbe · 2017-02-22T02:40:58Z

This is actually a super simple change now - most of the work has been done. Would be awesome to get this merged in soon! Means that anyone can fully build Swift on Windows, when #7626 and #7588 (but not necessarily run compiled code fully until #7489)

compnerd requested changes Nov 27, 2016

View reviewed changes

compnerd previously requested changes Nov 27, 2016

View reviewed changes

modocache reviewed Nov 28, 2016

View reviewed changes

BasThomas mentioned this pull request Nov 28, 2016

[48] Issue #48, Dec 1st SwiftWeekly/swiftweekly.github.io#142

Closed

hughbe mentioned this pull request Nov 28, 2016

Port swift/basic to Windows using MSVC #5949

Merged

bredov mentioned this pull request Nov 28, 2016

Swift Support microsoft/WinObjC#25

Open

hughbe mentioned this pull request Nov 28, 2016

Fix errors and warnings build swift/clangImporter using MSVC on Windows #5950

Merged

jrose-apple reviewed Nov 29, 2016

View reviewed changes

llvm-beanz reviewed Nov 30, 2016

View reviewed changes

jrose-apple reviewed Nov 30, 2016

View reviewed changes

jrose-apple reviewed Dec 1, 2016

View reviewed changes

llvm-beanz reviewed Dec 2, 2016

View reviewed changes

evieluvsrainbows approved these changes Dec 19, 2016

View reviewed changes

hughbe mentioned this pull request Dec 22, 2016

Add instructions for building Swift on Windows using clang-cl/MSVC #6458

Merged

hughbe changed the title ~~Add support and instructions for building Swift on Windows with MSVC~~ Add support for building Swift on Windows with MSVC Dec 22, 2016

hughbe changed the title ~~Add support for building Swift on Windows with MSVC~~ Add support for building Swift on Windows with clang-cl and MSVC Dec 22, 2016

hughbe mentioned this pull request Jan 18, 2017

Don't build swiftMSVCRT with MSVC #6843

Closed

hughbe requested a review from compnerd January 19, 2017 08:52

hughbe mentioned this pull request Feb 8, 2017

Add documentation for generating a Visual Studio project on Windows #7248

Merged

Fix misc compiler flags passed to MSVC/clang-cl

3d53967

hughbe requested review from jrose-apple and jckarter February 22, 2017 02:41

jrose-apple approved these changes Feb 23, 2017

View reviewed changes

hughbe merged commit 99f9581 into swiftlang:master Feb 24, 2017

hughbe deleted the windows-cmake branch February 24, 2017 00:52

		# CMake doesn't support create_symlink on Windows: http://cmake.3232098.n2.nabble.com/cmake-E-create-symlink-for-Windows-td4151271.html
		# TODO(hughbe/Windows): Windows does not support libedit currently. See the TODO in tools/SourceKit/tools/CMakeLists.txt

Add support for building Swift on Windows with clang-cl and MSVC #5904

Add support for building Swift on Windows with clang-cl and MSVC #5904

Conversation

hughbe commented Nov 22, 2016 • edited Loading

Caveat

Discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Nov 22, 2016 •

edited

Loading

hughbe Nov 28, 2016 •

edited

Loading

hughbe Nov 28, 2016 •

edited

Loading

hughbe Nov 28, 2016 •

edited

Loading