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

Documentation: Update CMake.md to use the ABI entry point #828

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ADKaster
Copy link
Contributor

@ADKaster ADKaster commented Nov 17, 2024

The SwiftPM entry point is unstable and the new ABIv0 entry point has already been added to the library.

Motivation:

Using the SwiftPM entry point when building tests from a CMake project as recommended in the documentation is outdated and unwise.

Modifications:

Replace the example with one using the new ABIv0 entry point.

Result:

CMake projects should stop relying on the SwiftPM entry point.

Checklist:

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

The SwiftPM entry point is unstable and the new ABIv0 entry point has
already been added to the library.
@ADKaster
Copy link
Contributor Author

cc @compnerd

@grynspan grynspan added bug Something isn't working documentation Improvements or additions to documentation tools integration Integration of swift-testing into tools/IDEs build Affects the project's build configuration or process labels Nov 17, 2024
@ADKaster
Copy link
Contributor Author

Hm. Chatting with Jonathan on Slack some more, it seems like the fact that this works with the Swift OSS toolchain on linux is a toolchain bug: we shouldn't be able to access the private module from user code.

Another alternative (with -enable-experimental-feature Extern) looks like this:

import Foundation

typealias EntryPoint = @convention(thin) @Sendable (_ configurationJSON: UnsafeRawBufferPointer?, _ recordHandler: @escaping @Sendable (_ recordJSON: UnsafeRawBufferPointer) -> Void) async throws -> Bool

@_extern(c, "swt_abiv0_getEntryPoint")
func swt_abiv0_getEntryPoint() -> UnsafeRawPointer

@main struct Runner {
    static func main() async throws {
        let configurationJSON: UnsafeRawBufferPointer? = nil
        let recordHandler: @Sendable (UnsafeRawBufferPointer) -> Void = { _ in }

        let entryPoint = unsafeBitCast(swt_abiv0_getEntryPoint(), to: EntryPoint.self)

        if try await entryPoint(configurationJSON, recordHandler) {
            exit(EXIT_SUCCESS)
        } else {
            exit(EXIT_FAILURE)
        }
    }
}

Documentation/CMake.md Outdated Show resolved Hide resolved
Documentation/CMake.md Outdated Show resolved Hide resolved
@grynspan
Copy link
Contributor

@ADKaster Did you want to proceed with this PR?

@ADKaster
Copy link
Contributor Author

@grynspan I've updated the PR to address your two comments and other discussions we had on slack. I'm not 100% sure on the formatting, but the example code as-is does work.


```swift
import Testing
import Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need this import for the barebones implementation below. Only needed if you want to use JSONEncoder/JSONDecoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it not needed for exit(EXIT_SUCCESS)? Or is there a non-foundation way to do the standard Unix process exit dance.

Copy link
Contributor

Choose a reason for hiding this comment

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

exit() and EXIT_SUCCESS are defined in the system's C module, which may be Darwin, Glibc, ucrt… uh… Musl?

Foundation reexports the C module for the current system. It seems wrong to me (but I can be convinced otherwise) to tell developers to include a full Foundation dependency just for exit().

Depending on the dev's specific requirements, they really just need the process to terminate with zero (the default!) or non-zero. If they're using Swift Argument Parser, it has built-in functionality for this. If they're using just the Swift stdlib and nothing else, they could call fatalError() on failure. I don't know what's the best choice for this example code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh.. I'm not sure that I know what the best choice is either. When a test binary is executed by ctest, as in my use case, returning 0 or 1 from main() is important for the test harness to understand whether the test passed or failed. Anything more than that on the ctest side is up to whatever @etcwilde comes up with for https://gitlab.kitware.com/cmake/cmake/-/issues/26452

I feel like adding a

#if canImport(GlibC)
#import GlibC
#elseif canImport(YourCLibModule)
#import YourCLibModule
#endif

chain to the top of the example would get in the way of what it's actually trying to show that you need 🤷

I'll change it to whatever anyone feels strongly about though, as in theory more comprehensive support for the CMake use case could come soon ™️ to make this example obsolete-ish.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may be sufficient to just show exit(EXIT_SUCCESS) without importing anything. Developers who are going down this path are presumably skilled enough to figure out how to make it compile…? 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure... "compilable example code left as an exercise for the reader" is definitely one approach. Not my first choice, but it's not my library :). I've pushed up that change then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, anybody who's digging down into the CMake scripts is on their own and I have to assume has some esoteric environment—we can't even guarantee Foundation is present! (We can guarantee libc is present because we won't build without it, at least.)

> The entry point is expected to change to an entry point designed for other
> build systems prior to the initial stable release of Swift Testing.
For more information on the input configuration and output records of the ABI entry
point, refer to the [ABI documentation](ABI/JSON.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
point, refer to the [ABI documentation](ABI/JSON.md)
point, refer to the [ABI documentation](ABI/JSON.md).

Copy link
Contributor

@stmontgomery stmontgomery left a comment

Choose a reason for hiding this comment

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

I'd like to see at least a code comment related to failure tracking leading to a non-zero exit status. Bonus points for actually implementing that logic in the example code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build Affects the project's build configuration or process documentation Improvements or additions to documentation tools integration Integration of swift-testing into tools/IDEs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants