[CMake] Fix build with SwiftTesting_BuildMacrosAsExecutables option set

ADKaster commented 3 weeks ago

Resolve the issues with an out-of-toolchain build from the command line with ninja

Motivation:

Fixes https://github.com/swiftlang/swift-testing/issues/644

Modifications:

There were two issues:

typo -load-plugin-exectuable -> -load-plugin-executable
The executable target for TestingMacros was not installed

Result:

A build from the command line with cmake -GNinja -Bbuild will successfully complete if SwiftSyntax is not find_package-able

Checklist:

[x] Code and documentation should follow the style of the Style Guide.
[x] If public symbols are renamed or modified, DocC references should be updated.

ADKaster commented 3 weeks ago

cc @compnerd @etcwilde

grynspan commented 3 weeks ago

When building the toolchain, the macro product goes somewhere… so I'd expect to put it in the same place. :)

ADKaster commented 3 weeks ago

It looks to me like when @rintaro added the install rules in https://github.com/swiftlang/swift-testing/pull/581 , the intention was for executables to be installed into ${CMAKE_INSTALL_BINDIR} and/or bin/ . The install command in the else() branch below (line 77/80) and in cmake/modules/SwiftModuleInstallation.cmake both point there for RUNTIME artifacts.

I also don't know what the toolchain does for the macros regarding libs vs executables though, that's on y'all :).

etcwilde commented 2 weeks ago

So for the toolchain builds, the macros are built entirely separately. They're kind of awkward when dealing with cross-compiling as well since they need to build for the build machine of the people building against the SDK rather than what the SDK is built to run on. Still trying to figure out exactly how to ship these things reasonably.

I've personally been using FetchContent to merge the build graphs of the projects, which then means that I pick up the macro plugin path when I link something against the Testing target, though that naming could certainly conflict. Just so that I understand your goal and the issue you're running into, are you using ExternalProject_Add and trying to plumb through the location of the macro through the imported swift-testing target? If that's the case, I could certainly see this being an issue.

I'm also going to ping @compnerd, @rintaro, and @al45tair for thoughts since I think it makes more sense to install the macros to libexec/ than to bin/ on unix-y systems. To be clear, I'm not saying you need to figure that out or do anything, just that we need an install story for macros.

ADKaster commented 2 weeks ago

are you using ExternalProject_Add and trying to plumb through the location of the macro through the imported swift-testing target?

Yes, my pull request adding it via FetchContent is here: https://github.com/LadybirdBrowser/ladybird/pull/1202

Note that I did run into two separate issues with that (beyond the typo).

The first is that the Testing target would not build unless the TestingMacros target was installed, as I am not including SwiftSyntax into my project (yet) and swift-testing builds the TestingMacros target itself as an executable if it can't be find_package'd. That's fixed in this PR by adding the install() rule.

The second is that the Testing target didn't actually include any interface properties that instruct CMake to use the macro plugin, so I had to add my own target property to it before exposing it to my CMake build for consumers to use, like so:

set_property(TARGET Testing APPEND PROPERTY INTERFACE_COMPILE_OPTIONS "$<$<COMPILE_LANGUAGE:Swift>:SHELL:-load-plugin-executable ${CMAKE_BINARY_DIR}/bin/TestingMacros#TestingMacros>")

So if using this project via FetchContent is a goal, adding that automagically would be nice :).

which then means that I pick up the macro plugin path when I link something against the Testing target, though that naming could certainly conflict.

In light of that second issue, I'm not sure if I'm holding it wrong, or if you did some other magic to pick up the plugin when linking against Testing 🤔 .

As a side note, I did make two additional changes in my .patch file when pulling swift-testing in via FetchContent:

Marking TestingMacros as not BUILD_ALWAYS YES. Perhaps a patch to change the "YES" to "${PROJECT_IS_TOP_LEVEL}" would make sense? if I'm not modifying Testing, rebuilding the macros is annoying and takes tens of seconds.
Using the USES_TERMINAL_{CONFIGURE, BUILD, INSTALL} properties on the FetchContent'd projects when using Ninja. This adds the jobs for the external projects to the "Console" job pool and prevents their output from being stored in a buffer and splatted onto my terminal screen with newlines, and lets it co-exist with the nice one-line output ninja is good at.

grynspan commented 2 weeks ago

So for comparison's sake, we already have:

    if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")
      set(SwiftTesting_MACRO_PATH "${SwiftTesting_MACRO_INSTALL_PREFIX}/bin/TestingMacros.exe")
    else()
      set(SwiftTesting_MACRO_PATH "${SwiftTesting_MACRO_INSTALL_PREFIX}/bin/TestingMacros")
    endif()

So… as long as we're consistent with the rest of our CMake script, I'm happy with the proposed changes. If @etcwilde is happy with them, we can run CI and then merge this change.

rintaro commented 2 weeks ago

I think it makes more sense to install the macros to libexec/ than to bin/ on unix-y systems. To be clear, I'm not saying you need to figure that out or do anything, just that we need an install story for macros.

I don't have strong opinion on where to install macro plugin executables, this is just an install from ExternalProject to the CMake's build directory. As long as it's accessible and consistent, I'm fine.

Marking TestingMacros as not BUILD_ALWAYS YES. Perhaps a patch to change the "YES" to "${PROJECT_IS_TOP_LEVEL}" would make sense? if I'm not modifying Testing, rebuilding the macros is annoying and takes tens of seconds.

BUILD_ALWAYS YES because ExternalProject doesn't pick up the changes in the project. If nothing has changed, Ninja should treat it as up-to-date. But we know that's not the case by default. I believe we need CMP0157 enabled. cc: @etcwilde

etcwilde commented 2 weeks ago

I don't have strong opinion on where to install macro plugin executables, this is just an install from ExternalProject to the CMake's build directory. As long as it's accessible and consistent, I'm fine.

Okay, as long as we can come back and align things a little better with the filesystem hierarchy standard on Linux/Unix later, I'm fine with this as-is to get things unblocked.

BUILD_ALWAYS YES because ExternalProject doesn't pick up the changes in the project. If nothing has changed, Ninja should treat it as up-to-date. But we know that's not the case by default. I believe we need CMP0157 enabled.

I think I agree with switching it to ${PROJECT_IS_TOP_LEVEL} or exposing an option defaulted to OFF (or ${PROJECT_IS_TOP_LEVEL}) for forcing it to rebuild the macros. Folks who aren't actively iterating on the macros shouldn't need to rebuild them every time. I'm not 100% on exactly what is going on here and why it's always rebuilding, but I have a couple of suspicions. One, there's a target that forces a rebuild of the swiftmodules in SwiftSyntax due to improper dependency tracking (fix in CMake 3.26.0). We'll be able to remove the forced swift-syntax rebuilds once we require CMake 3.26 or newer which should help. Two, the Swift driver doesn't update files unless changes to the source actually require changes to the file (e.g. a change to a comment in a source file won't update the swiftmodule or objects), so ninja sees that as the output being out-of-date. The new build model in CMake 3.29 fixes that and that requires CMP0157.

grynspan commented 2 weeks ago

@swift-ci test

grynspan commented 2 weeks ago

@ADKaster You are free to merge this PR once all CI jobs complete. I recommend a squash-and-merge.

ADKaster commented 2 weeks ago

@grynspan as I'm not a member of swiftlang or granted write access on this repository specifically, I'm gonna have to ask that someone else push the merge button

grynspan commented 2 weeks ago

I'm never sure who can see the green button! Sorry!

al45tair commented 2 weeks ago

I'm also going to ping @compnerd, @rintaro, and @al45tair for thoughts since I think it makes more sense to install the macros to libexec/ than to bin/ on unix-y systems.

Agreed, they should really live in libexec (which is where swift-backtrace lives as well, as it happens, so we are now using libexec instead of shoving everything in lib).

compnerd commented 2 weeks ago

I'm also going to ping @compnerd, @rintaro, and @al45tair for thoughts since I think it makes more sense to install the macros to libexec/ than to bin/ on unix-y systems.

Agreed, they should really live in libexec (which is where swift-backtrace lives as well, as it happens, so we are now using libexec instead of shoving everything in lib).

I'm not sure about that, FHS dictates that libexec is for executable. See https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html Note that binaries here is not a generic term but rather used as executable (.exe on Windows). IMO the plug-ins should go under /usr/lib/swift/plugins or if we can make it architecture independent,/usr/share/swift/plugins.

etcwilde commented 2 weeks ago

I'm not sure about that, FHS dictates that libexec is for executable

This is for executable macros, not the library macros.

al45tair commented 2 weeks ago

I'm also going to ping @compnerd, @rintaro, and @al45tair for thoughts since I think it makes more sense to install the macros to libexec/ than to bin/ on unix-y systems.

Agreed, they should really live in libexec (which is where swift-backtrace lives as well, as it happens, so we are now using libexec instead of shoving everything in lib).

I'm not sure about that, FHS dictates that libexec is for executable. See https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html Note that binaries here is not a generic term but rather used as executable (.exe on Windows). IMO the plug-ins should go under /usr/lib/swift/plugins or if we can make it architecture independent,/usr/share/swift/plugins.

My understanding is that the macros we're talking about here are executables, which is what libexec is for. If they were libraries then I agree /usr/lib/swift/plugins or similar might be a nice place. Definitely not bin in either case though.

compnerd commented 2 weeks ago

I'm also going to ping @compnerd, @rintaro, and @al45tair for thoughts since I think it makes more sense to install the macros to libexec/ than to bin/ on unix-y systems.

Agreed, they should really live in libexec (which is where swift-backtrace lives as well, as it happens, so we are now using libexec instead of shoving everything in lib).

I'm not sure about that, FHS dictates that libexec is for executable. See https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html Note that binaries here is not a generic term but rather used as executable (.exe on Windows). IMO the plug-ins should go under /usr/lib/swift/plugins or if we can make it architecture independent,/usr/share/swift/plugins.

My understanding is that the macros we're talking about here are executables, which is what libexec is for. If they were libraries then I agree /usr/lib/swift/plugins or similar might be a nice place. Definitely not bin in either case though.

Ah, the plugins built as executables definitely belong in libexec, but when built as DSOs, I think that lib is more appropriate.

grynspan commented 2 weeks ago

Ah, the plugins built as executables definitely belong in libexec, but when built as DSOs, I think that lib is more appropriate.

That makes sense to me.

swiftlang / swift-testing