swiftlang / swift-source-compat-suite

The infrastructure and project index comprising the Swift source compatibility suite.
Apache License 2.0
284 stars 150 forks source link

Add vapor/penny-bot #880

Closed MahdiBM closed 8 months ago

MahdiBM commented 9 months ago

Pull Request Description

Add Penny to the source-compatibility list. Penny is Vapor's automation bot. Penny is compatible with the latest swift changes and upcoming flags (sometimes experimental too) so can be valuable in that aspect (flags visible in Package.swift). Furthermore Penny is a project deployed on Linux environments (currently on amazonlinux2 Swift images), so can be used by the source compatibility team as a means to ensure latest Swift changes are compatible with real-world Server-Side Swift apps deployed on Linux and AWS.

Acceptance Criteria

To be accepted into the Swift source compatibility test suite, a project must:

Ensure project meets all listed requirements before submitting a pull request.

MahdiBM commented 9 months ago

@0xTim @gwynne I want an approval from you as well πŸ™‚ (Just a πŸ‘ can be enough)

0xTim commented 9 months ago

LGTM

justice-adams-apple commented 9 months ago

@swift-ci test

justice-adams-apple commented 9 months ago

πŸ‘‹ @MahdiBM thanks for the PR. does penny-bot build in release configuration? I seem to be seeing an error on CI and locally

penny-bot/Tests/Fake/+Endpoint.swift:1:18: error: module 'DiscordHTTP' was not compiled for testing
@testable import DiscordHTTP

Reproducer: xcrun swift build --configuration release

If not we'll need to account for that by xfailing the release configuration since we run nightly jobs for both debug and release builds of all projects

MahdiBM commented 9 months ago

@justice-adams-apple πŸ‘‹. Thanks for investigating this.

I did notice the issue reading the CI logs. It seems like some kind of compiler bug or inability to me.

To answer your question more directly, yes, Penny does build with release configuration. We use the release configuration for deployments, like we should. We run the tests separately however, so the --build-tests part is not something we usually do.

I will resolve the issue one way or another, over the weekend if not sooner.

justice-adams-apple commented 8 months ago

@MahdiBM poked around, it seems this is due to the fact that the source compat suite runs

swift build --configuration release

to build swiftPM projects. We don't necessarily target a specific product (e.g.)

swift build --configuration release -product Penny

And it seems you may have a target which is declared as a target and not testTarget and so swiftpm tries to build it appropriately. https://github.com/vapor/penny-bot/blob/main/Package.swift#L261

I suspect that may be intended to be declared as a testTarget but correct me if i'm wrong

MahdiBM commented 8 months ago

@justice-adams-apple there is a target called 'Fake' which is only used in a test target. Swift tries to build that target in release mode, and @testable imports are not available there which results in the problem.

The target is test related, but can't be marked as a test target due to a Xcode error IIRC. At the same time, there is this problem when declaring it as a normal target.

I think using '-Xswiftc -enable-testing' would solve the problem, but I also think it's not supported here in the source compat suite.

It's not a big deal anyway and I've moved the 'Fake' target into the related test target in the PR linked to this issue. That should solve the problem, but I'm waiting on Tim or Gwynne to give it a review first, before merging it.

MahdiBM commented 8 months ago

@justice-adams-apple try again now please πŸ™‚

Reading the projects.py script, I noticed the source compat suite does use -enable-testing, which is good and means we should be able to have the build_tests flag enabled too.

justice-adams-apple commented 8 months ago

Reading the projects.py script, I noticed the source compat suite does use -enable-testing, which is good and means we should be able to have the build_tests flag enabled to

πŸ‘ Thanks for the update! Will try to get this in asap πŸ˜„

justice-adams-apple commented 8 months ago

@swift-ci test