Skip to content

[CMake] Localize a build hack #12895

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 15, 2017
Merged

[CMake] Localize a build hack #12895

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
4 changes: 3 additions & 1 deletion cmake/modules/SwiftSharedCMakeConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ macro(swift_common_standalone_build_config_llvm product is_cross_compiling)
# HACK: this ugly tweaking is to prevent the propagation of the flag from LLVM
# into swift. The use of this flag pollutes all targets, and we are not able
# to remove it on a per-target basis which breaks cross-compilation.
string(REGEX REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
if("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
Copy link
Member

@compnerd compnerd Nov 15, 2017

Choose a reason for hiding this comment

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

This is wrong. This flag only comes in cross compiling. The CMAKE_SYSTEM_NAME is UNIX.

Copy link
Contributor Author

@davezarzycki davezarzycki Nov 15, 2017

Choose a reason for hiding this comment

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

I'm happy to refine this. What is the right way to check for Windows in this case?

Copy link
Member

@compnerd compnerd Nov 16, 2017

Choose a reason for hiding this comment

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

You cannot at this level, hence I was suggesting that we sink this further. Pushing this into the add_library call would be sufficiently low that we can check the target for the library being built.

Copy link
Contributor Author

@davezarzycki davezarzycki Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is it really necessary on Windows to modify add_library? What specific Swift binaries need this flag removed? For example, on Linux only SwiftRemoteMirror "needs" the flag removed, and after I looked into the scenario, I'm convinced that -Wl,-z,defs is correctly identifying a bug, so removing the flag only makes the problem worse, not better.

Rather than add infrastructure to formalize the removal of -Wl,-z,defs, it seems better/wiser to contain the problem to the affected libraries until they can be cleaned up.

Copy link
Member

@compnerd compnerd Nov 16, 2017

Choose a reason for hiding this comment

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

Yes, it really is necessary to modify add_library. The point is that we are sharing the flags incorrectly. The flag is for build not for host when cross-compiling. Im compiling a compiler for Linux and the stdlib and tests for Windows. The flags are not valid for the host, but are valid for the build. It is any library that is going to run on the host that needs the alteration. This would include things like the stdlib, the runtime, SwiftDemangle, SwiftRemoteMirror, etc.

string(REGEX REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
endif()

set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
string(REGEX REPLACE "([0-9]+)\\.[0-9]+(\\.[0-9]+)?" "\\1" PACKAGE_VERSION_MAJOR
Expand Down
3 changes: 3 additions & 0 deletions stdlib/public/SwiftRemoteMirror/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# HACK: Force this library to build with undefined symbols.
string(REGEX REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")

# libswiftRemoteMirror.dylib should not have runtime dependencies; it's
# always built as a shared library.
if(SWIFT_BUILD_DYNAMIC_STDLIB OR SWIFT_BUILD_REMOTE_MIRROR)
Expand Down