Skip to content

Change stdlib swiftc's to depend on .swiftmodule's instead of .o's #5472

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

Merged
merged 1 commit into from
Nov 1, 2016
Merged

Change stdlib swiftc's to depend on .swiftmodule's instead of .o's #5472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions cmake/modules/AddSwift.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ endfunction()
# _add_swift_library_single(
# target
# name
# [MODULE_TARGET]
# [SHARED]
# [STATIC]
# [SDK sdk]
Expand Down Expand Up @@ -474,6 +475,9 @@ endfunction()
# name
# Name of the library (e.g., swiftParse).
#
# MODULE_TARGET
# Name of the module target (e.g., swiftParse-swiftmodule-IOS-armv7).
#
# SHARED
# Build a shared library.
#
Expand Down Expand Up @@ -543,7 +547,7 @@ function(_add_swift_library_single target name)
API_NOTES_NON_OVERLAY DONT_EMBED_BITCODE)
cmake_parse_arguments(SWIFTLIB_SINGLE
"${SWIFTLIB_SINGLE_options}"
"SDK;ARCHITECTURE;INSTALL_IN_COMPONENT;DEPLOYMENT_VERSION_IOS"
"MODULE_TARGET;SDK;ARCHITECTURE;INSTALL_IN_COMPONENT;DEPLOYMENT_VERSION_IOS"
"DEPENDS;LINK_LIBRARIES;FRAMEWORK_DEPENDS;FRAMEWORK_DEPENDS_WEAK;LLVM_COMPONENT_DEPENDS;C_COMPILE_FLAGS;SWIFT_COMPILE_FLAGS;LINK_FLAGS;PRIVATE_LINK_LIBRARIES;INTERFACE_LINK_LIBRARIES;INCORPORATE_OBJECT_LIBRARIES;FILE_DEPENDS"
${ARGN})

Expand Down Expand Up @@ -682,6 +686,7 @@ function(_add_swift_library_single target name)
# just any swiftmodule files that are associated with them.
handle_swift_sources(
swift_object_dependency_target
swift_module_dependency_target
SWIFTLIB_SINGLE_SOURCES
SWIFTLIB_SINGLE_EXTERNAL_SOURCES ${name}
DEPENDS
Expand All @@ -700,6 +705,13 @@ function(_add_swift_library_single target name)
INSTALL_IN_COMPONENT "${SWIFTLIB_INSTALL_IN_COMPONENT}")
add_swift_source_group("${SWIFTLIB_SINGLE_EXTERNAL_SOURCES}")

# If there were any swift sources, then a .swiftmodule may have been created.
# If that is the case, then add a target which is an alias of the module files.
if(NOT "${SWIFTLIB_SINGLE_MODULE_TARGET}" STREQUAL "" AND NOT "${swift_module_dependency_target}" STREQUAL "")
add_custom_target("${SWIFTLIB_SINGLE_MODULE_TARGET}"
DEPENDS ${swift_module_dependency_target})
endif()

set(VARIANT_SUFFIX "-${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}-${SWIFTLIB_SINGLE_ARCHITECTURE}")
set(SWIFTLIB_INCORPORATED_OBJECT_LIBRARIES_EXPRESSIONS)
foreach(object_library ${SWIFTLIB_SINGLE_INCORPORATE_OBJECT_LIBRARIES})
Expand Down Expand Up @@ -874,6 +886,7 @@ function(_add_swift_library_single target name)
${SWIFTLIB_SINGLE_DEPENDS}
${gyb_dependency_targets}
"${swift_object_dependency_target}"
"${swift_module_dependency_target}"
${LLVM_COMMON_DEPENDS})

# HACK: On some systems or build directory setups, CMake will not find static
Expand Down Expand Up @@ -1320,6 +1333,8 @@ function(add_swift_library name)
# Configure variables for this subdirectory.
set(VARIANT_SUFFIX "-${SWIFT_SDK_${sdk}_LIB_SUBDIR}-${arch}")
set(VARIANT_NAME "${name}${VARIANT_SUFFIX}")
set(MODULE_VARIANT_SUFFIX "-swiftmodule${VARIANT_SUFFIX}")
set(MODULE_VARIANT_NAME "${name}${MODULE_VARIANT_SUFFIX}")

# Map dependencies over to the appropriate variants.
set(swiftlib_link_libraries)
Expand Down Expand Up @@ -1352,23 +1367,17 @@ function(add_swift_library name)
${SWIFTLIB_SWIFT_MODULE_DEPENDS_LINUX})
endif()

# Swift compiles depend on swift modules, while links depend on
# linked libraries. Find targets for both of these here.
set(swiftlib_module_dependency_targets)
set(swiftlib_private_link_libraries_targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am missing something, isn't the variable swiftlib_private_link_libraries_targets dead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't see to be. I had issues with this when I was writing the patch. Ultimately it makes its way to the following which causes dylibs to be on the link line:

function(_add_swift_library_single
...
target_link_libraries("${target}" PRIVATE
${SWIFTLIB_SINGLE_PRIVATE_LINK_LIBRARIES})

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it's still a dependency of the link target for each module.

foreach(mod ${swiftlib_module_depends_flattened})
list(APPEND swiftlib_module_dependency_targets
"swift${mod}${MODULE_VARIANT_SUFFIX}")
list(APPEND swiftlib_private_link_libraries_targets
"swift${mod}${VARIANT_SUFFIX}")
endforeach()

set(swiftlib_framework_depends_flattened ${SWIFTLIB_FRAMEWORK_DEPENDS})
if("${sdk}" STREQUAL "OSX")
list(APPEND swiftlib_framework_depends_flattened
${SWIFTLIB_FRAMEWORK_DEPENDS_OSX})
elseif("${sdk}" STREQUAL "IOS" OR "${sdk}" STREQUAL "IOS_SIMULATOR" OR "${sdk}" STREQUAL "TVOS" OR "${sdk}" STREQUAL "TVOS_SIMULATOR")
list(APPEND swiftlib_framework_depends_flattened
${SWIFTLIB_FRAMEWORK_DEPENDS_IOS_TVOS})
endif()

set(swiftlib_private_link_libraries_targets
${swiftlib_module_dependency_targets})
foreach(lib ${SWIFTLIB_PRIVATE_LINK_LIBRARIES})
if("${lib}" STREQUAL "ICU_UC")
list(APPEND swiftlib_private_link_libraries_targets
Expand All @@ -1384,6 +1393,15 @@ function(add_swift_library name)
endif()
endforeach()

set(swiftlib_framework_depends_flattened ${SWIFTLIB_FRAMEWORK_DEPENDS})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move this?

Copy link
Contributor

Choose a reason for hiding this comment

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

And if it is just a move, can you do the move in a separate commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it didn't move. It the diff not really knowing how I applied the changes.

What we have before was a loop to set swiftlib_module_dependency_targets, then later a loop to set swiftlib_framework_depends_flattened, and finally a loop for swiftlib_private_link_libraries_targets.

In the original code, swiftlib_private_link_libraries_targets was initialized to swiftlib_module_dependency_targets, but we don't want that any more. This is because swiftlib_module_dependency_targets now contains references to the swift module files, and we want swiftlib_private_link_libraries_targets to reference the linked images. So I changed swiftlib_private_link_libraries_targets to initialise to empty instead of swiftlib_module_dependency_targets then added appends to this loop to set it appropriately.

And yes, I could definitely move the in a separate commit then only make the change the following a change in a new commit:

list(APPEND swiftlib_module_dependency_targets
"swift${mod}${VARIANT_SUFFIX}")

to

list(APPEND swiftlib_module_dependency_targets
"swift${mod}${MODULE_VARIANT_SUFFIX}")

Copy link
Contributor

Choose a reason for hiding this comment

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

There definitely is a move here: the ICU section and the swiftlib_framework_depends_flattened switched. I do see that it makes a little more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yes there's a move. I still had the 'git diff' my local machine shows in my head. In it, it appears that the loop over swiftlib_framework_depends_flattened moves, which it didn't really. What actually moved was the content I described above.

if("${sdk}" STREQUAL "OSX")
list(APPEND swiftlib_framework_depends_flattened
${SWIFTLIB_FRAMEWORK_DEPENDS_OSX})
elseif("${sdk}" STREQUAL "IOS" OR "${sdk}" STREQUAL "IOS_SIMULATOR" OR "${sdk}" STREQUAL "TVOS" OR "${sdk}" STREQUAL "TVOS_SIMULATOR")
list(APPEND swiftlib_framework_depends_flattened
${SWIFTLIB_FRAMEWORK_DEPENDS_IOS_TVOS})
endif()

# Collect compiler flags
set(swiftlib_swift_compile_flags_all ${SWIFTLIB_SWIFT_COMPILE_FLAGS})
if("${sdk}" STREQUAL "OSX")
Expand Down Expand Up @@ -1421,6 +1439,7 @@ function(add_swift_library name)
${SWIFTLIB_STATIC_keyword}
${SWIFTLIB_OBJECT_LIBRARY_keyword}
${SWIFTLIB_SOURCES}
MODULE_TARGET ${MODULE_VARIANT_NAME}
SDK ${sdk}
ARCHITECTURE ${arch}
DEPENDS ${SWIFTLIB_DEPENDS}
Expand Down Expand Up @@ -1730,6 +1749,7 @@ function(_add_swift_executable_single name)

handle_swift_sources(
dependency_target
unused_module_dependency_target
SWIFTEXE_SINGLE_SOURCES SWIFTEXE_SINGLE_EXTERNAL_SOURCES ${name}
DEPENDS
${SWIFTEXE_SINGLE_DEPENDS}
Expand Down
89 changes: 70 additions & 19 deletions cmake/modules/SwiftSource.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
# Usage:
# handle_swift_sources(sourcesvar externalvar)
function(handle_swift_sources
dependency_target_out_var_name sourcesvar externalvar name)
dependency_target_out_var_name
dependency_module_target_out_var_name
sourcesvar externalvar name)
cmake_parse_arguments(SWIFTSOURCES
"IS_MAIN;IS_STDLIB;IS_STDLIB_CORE;IS_SDK_OVERLAY"
"SDK;ARCHITECTURE;INSTALL_IN_COMPONENT"
Expand Down Expand Up @@ -38,6 +40,7 @@ function(handle_swift_sources

# Clear the result variable.
set("${dependency_target_out_var_name}" "" PARENT_SCOPE)
set("${dependency_module_target_out_var_name}" "" PARENT_SCOPE)

set(result)
set(swift_sources)
Expand Down Expand Up @@ -78,6 +81,7 @@ function(handle_swift_sources

_compile_swift_files(
dependency_target
module_dependency_target
OUTPUT ${swift_obj}
SOURCES ${swift_sources}
DEPENDS ${SWIFTSOURCES_DEPENDS}
Expand All @@ -93,6 +97,7 @@ function(handle_swift_sources
${STATIC_arg}
INSTALL_IN_COMPONENT "${SWIFTSOURCES_INSTALL_IN_COMPONENT}")
set("${dependency_target_out_var_name}" "${dependency_target}" PARENT_SCOPE)
set("${dependency_module_target_out_var_name}" "${module_dependency_target}" PARENT_SCOPE)
list(APPEND result ${swift_obj})
endif()

Expand Down Expand Up @@ -134,7 +139,8 @@ endfunction()
# [IS_STDLIB] # Install produced files.
# [EMIT_SIB] # Emit the file as a sib file instead of a .o
# )
function(_compile_swift_files dependency_target_out_var_name)
function(_compile_swift_files
dependency_target_out_var_name dependency_module_target_out_var_name)
cmake_parse_arguments(SWIFTFILE
"IS_MAIN;IS_STDLIB;IS_STDLIB_CORE;IS_SDK_OVERLAY;EMIT_SIB"
"OUTPUT;MODULE_NAME;INSTALL_IN_COMPONENT"
Expand Down Expand Up @@ -274,6 +280,7 @@ function(_compile_swift_files dependency_target_out_var_name)
set(module_file)
set(module_doc_file)

set(module_command)
if(NOT SWIFTFILE_IS_MAIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

This section may make sense to refactor into its own function... thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

My reasoning is that in the general flow of things, adding swift_flags -parse-as-library where you adding it, seems funky. That being said, this isn't that big of a deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was actually just a move as the previous code had use adding '-emit-module ... -parse-as-library' on one branch and '-parse-as-library' on the other. But I agree that this is getting a little unwieldy at its current size. Thats probably worth someone refactoring a whole bunch of stuff though, which is better left to who is going to own it long term.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I am fine with that.

# Determine the directory where the module file should be placed.
if(SWIFTFILE_MODULE_DIR)
Expand All @@ -284,16 +291,15 @@ function(_compile_swift_files dependency_target_out_var_name)
message(FATAL_ERROR "Don't know where to put the module files")
endif()

list(APPEND swift_flags "-parse-as-library")
if (NOT SWIFTFILE_EMIT_SIB)
# Right now sib files seem to not be output when we emit a module. So
# don't emit it.
set(module_file "${module_dir}/${SWIFTFILE_MODULE_NAME}.swiftmodule")
set(module_doc_file "${module_dir}/${SWIFTFILE_MODULE_NAME}.swiftdoc")
list(APPEND swift_flags
"-parse-as-library"
"-emit-module" "-emit-module-path" "${module_file}")
else()
list(APPEND swift_flags "-parse-as-library")
list(APPEND module_command
"-emit-module"
"-o" "${module_file}")
endif()

list(APPEND command_create_dirs
Expand Down Expand Up @@ -366,9 +372,9 @@ function(_compile_swift_files dependency_target_out_var_name)
set(swift_compiler_tool "${SWIFT_SOURCE_DIR}/utils/check-incremental" "${swift_compiler_tool}")
endif()

set(outputs
${SWIFTFILE_OUTPUT} "${module_file}" "${module_doc_file}"
${apinote_files})
set(standard_outputs ${SWIFTFILE_OUTPUT})
set(apinotes_outputs ${apinote_files})
set(module_outputs "${module_file}" "${module_doc_file}")

if(XCODE)
# HACK: work around an issue with CMake Xcode generator and the Swift
Expand All @@ -383,29 +389,74 @@ function(_compile_swift_files dependency_target_out_var_name)
#
# To work around this issue we touch the output files so that their mtime
# always gets updated.
set(command_touch_outputs
COMMAND "${CMAKE_COMMAND}" -E touch ${outputs})
set(command_touch_standard_outputs
COMMAND "${CMAKE_COMMAND}" -E touch ${standard_outputs})
set(command_touch_apinotes_outputs
COMMAND "${CMAKE_COMMAND}" -E touch ${apinotes_outputs})
set(command_touch_module_outputs
COMMAND "${CMAKE_COMMAND}" -E touch ${module_outputs})
endif()

# First generate the obj dirs
add_custom_command_target(
dependency_target
obj_dirs_dependency_target
${command_create_dirs}
# Create API notes before compiling, because this will affect the APIs
# the overlay sees.
${command_create_apinotes}
COMMAND ""
OUTPUT ${obj_dirs}
COMMENT "Generating obj dirs for ${first_output}")

# Generate the api notes if we need them.
if (apinotes_outputs)
add_custom_command_target(
api_notes_dependency_target
# Create API notes before compiling, because this will affect the APIs
# the overlay sees.
${command_create_apinotes}
${command_touch_apinotes_outputs}
COMMAND ""
OUTPUT ${apinotes_outputs}
DEPENDS
${swift_compiler_tool_dep}
${depends_create_apinotes}
${obj_dirs_dependency_target}
COMMENT "Generating API notes ${first_output}")
endif()

# Then we can compile both the object files and the swiftmodule files
# in parallel in this target for the object file, and ...
add_custom_command_target(
dependency_target
COMMAND
"${line_directive_tool}" "${source_files}" --
"${swift_compiler_tool}" "${main_command}" ${swift_flags}
${output_option} "${source_files}"
${command_touch_outputs}
OUTPUT ${outputs}
${command_touch_standard_outputs}
OUTPUT ${standard_outputs}
DEPENDS
${swift_compiler_tool_dep}
${source_files} ${SWIFTFILE_DEPENDS}
${swift_ide_test_dependency} ${depends_create_apinotes}
${swift_ide_test_dependency} ${api_notes_dependency_target}
${obj_dirs_dependency_target}
COMMENT "Compiling ${first_output}")
set("${dependency_target_out_var_name}" "${dependency_target}" PARENT_SCOPE)

# This is the target to generate the .swiftmodule and .swiftdoc
add_custom_command_target(
module_dependency_target
COMMAND
"${line_directive_tool}" "${source_files}" --
"${swift_compiler_tool}" "${module_command}" ${swift_flags}
"${source_files}"
${command_touch_module_outputs}
OUTPUT ${module_outputs}
DEPENDS
${swift_compiler_tool_dep}
${source_files} ${SWIFTFILE_DEPENDS}
${swift_ide_test_dependency} ${api_notes_dependency_target}
${obj_dirs_dependency_target}
COMMENT "Generating ${module_file}")
set("${dependency_module_target_out_var_name}" "${module_dependency_target}" PARENT_SCOPE)

# Make sure the build system knows the file is a generated object file.
set_source_files_properties(${SWIFTFILE_OUTPUT}
PROPERTIES
Expand Down