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

CMake: Fixing cross-compiling Swift-Foundation #878

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

etcwilde
Copy link
Contributor

The macros must build for the local machine running the build, not the machine that Swift-Foundation will run on, in order for the build to be able to use the macros.

To do this, the macro build must use ExternalProject, which treats the child project as an entirely independent project. One cannot introspect into the external project since it is configured at build time, rather than at configure time. This is what allows the external project to build for another platform.

The expectation is that the calling project will pull the built products of the ExternalProject from the install location. EPs have an internal implicit install prefix where they can install stuff to without dirtying the building machine or installed products, so we can make use of that. In order for that to work, the products must actually get installed though, so we have to install the FoundationMacros, even when built as an executable. To support that, I've exposed an option to tell the macro build to build the macros as an executable.

On the library side, I've exposed the Foundation macros as an interface library that only exposes the -load-plugin-path option needed for picking up the macro. Linking against this interface library will load the plugin as desired.

This results in a build that

  • can use macros, even when cross-compiling.
  • does not install the macros into the installed library, only to the build directory.

@etcwilde
Copy link
Contributor Author

@swift-ci please test

Sources/CMakeLists.txt Outdated Show resolved Hide resolved
CMAKE_ARGS
-DCMAKE_BUILD_TYPE=Release
-DCMAKE_Swift_COMPILER=${CMAKE_Swift_COMPILER}
-DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen the <INSTALL_DIR> syntax before, out of curiosity, what does it mean?

Copy link
Member

Choose a reason for hiding this comment

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

This is a special placeholder that will be filled in with the install path. This is documented in the ExternalProject documentation IIRC.

Sources/FoundationEssentials/CMakeLists.txt Outdated Show resolved Hide resolved
Sources/FoundationEssentials/CMakeLists.txt Outdated Show resolved Hide resolved
@etcwilde
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@jmschonfeld jmschonfeld left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise this looks good to me. Before merging, lets run a cross-repo CI test to get a full toolchain test going with this since this repo's CI doesn't test the cmake build yet

Sources/CMakeLists.txt Outdated Show resolved Hide resolved
@etcwilde
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@jmschonfeld jmschonfeld left a comment

Choose a reason for hiding this comment

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

Approved assuming cross-repo test comes back clean, thanks for updating this!

@jmschonfeld
Copy link
Contributor

-- SwiftSyntax_DIR not provided, checking out local copy of swift-syntax

Looks like we need to make sure SwiftSyntax_DIR gets passed along to the separate macro build

@etcwilde
Copy link
Contributor Author

etcwilde commented Aug 20, 2024

Looks like we need to make sure SwiftSyntax_DIR gets passed along to the separate macro build

Except that was built for the machine we're building the toolchain for, not the machine we're building the toolchain on.
(Assuming I'm reading build-script right)

We can, however, set <lowercaseName>_SOURCE_DIR so that we're not cloning it again. https://cmake.org/cmake/help/latest/module/FetchContent.html#command:fetchcontent_makeavailable

@jmschonfeld
Copy link
Contributor

Actually I got my wires crossed - in the toolchain build, swift-foundation should already receive SwiftFoundation_MACROS from the build script so it shouldn't ever hit the ExternalProject path, right?

Sources/CMakeLists.txt Outdated Show resolved Hide resolved
Sources/CMakeLists.txt Outdated Show resolved Hide resolved
@etcwilde
Copy link
Contributor Author

@swift-ci please test

The macros must build for the local machine running the build, not the
machine that Swift-Foundation will run on, in order for the build to be
able to use the macros.

To do this, the macro build must use ExternalProject, which treats the
child project as an entirely independent project. One cannot introspect
into the external project since it is configured at build time, rather
than at configure time. This is what allows the external project to
build for another platform.

The expectation is that the calling project will pull the built products
of the ExternalProject from the install location. EPs have an internal
implicit install prefix where they can install stuff to without dirtying
the building machine or installed products, so we can make use of that.
In order for that to work, the products must actually get installed
though, so we have to install the FoundationMacros, even when built as
an executable. To support that, I've exposed an option to tell the
macro build to build the macros as an executable.

On the library side, I've exposed the Foundation macros as an interface
library that only exposes the `-load-plugin-path` option needed for
picking up the macro. Linking against this interface library will load
the plugin as desired.

This results in a build that
   - can use macros, even when cross-compiling.
   - does not install the macros into the installed library, only to the
     build directory.
@etcwilde
Copy link
Contributor Author

@swift-ci please test

@finagolfin
Copy link
Contributor

finagolfin commented Sep 22, 2024

Is this really necessary? When cross-compiling this repo on the CI, you use a current freshly-built host toolchain from the same source, which already has these macros compiled and installed for the host. I haven't had any problem cross-compiling these new Foundation repos that way.

What I'd like instead is an option to disable cross-compiling these macros too, which is only useful for cross-compiling the toolchain, but not for standalone SDKs. For example, I'm cross-compiling an Android SDK using build-script right now and had to manually patch out cross-compiling these macros on there.

Btw @etcwilde, there appears to be a problem with building for Swift with the current CMake 3.30. You added the CMAKE_SHARED_MODULE_CREATE_Swift_FLAGS when building shared libraries, then set it for Apple-Swift alone later in that commit, which likely worked fine because that variable was uninitialized on other platforms.

However, a little later Brad default-initialized that variable for all languages to the C flags, which appears to have broken linking Swift libraries whenever the C flags are set on non-Apple platforms, by passing in C linker flags like -Wl,--build-id=sha1 to the Swift compiler. Never mind, that's not the reason. Comparing previous CMakeCache.txt files, these C linker flags were always set for CMAKE_SHARED_LINKER_FLAGS from the Android NDK, but CMake was careful to apply them only for C/C++ libraries like lib_FoundationICU.so and not for other language libraries like libFoundationEssentials.so.

However, something changed in the CMake source with 3.30 and it is now indiscriminately applying those C linker flags to all languages' shared libraries in mixed language CMake projects like this, breaking other languages like Swift.

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.

4 participants