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

Add vapor/penny-bot #880

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Add vapor/penny-bot #880

merged 3 commits into from
Jan 30, 2024

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Jan 20, 2024

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:

  • be an Xcode or swift package manager project
  • support building on either Linux or macOS
  • target Linux, macOS, or iOS/tvOS/watchOS device
  • be contained in a publicly accessible git repository
  • maintain a project branch that builds against Swift 4.0 5.9 and passes any unit tests
  • have maintainers who will commit to resolve issues in a timely manner
  • be compatible with the latest GM/Beta versions of Xcode and swiftpm
  • add value not already included in the suite
  • be licensed with one of the following permissive licenses:
    • BSD
    • MIT
    • Apache License, version 2.0
    • Eclipse Public License
    • Mozilla Public License (MPL) 1.1
    • MPL 2.0
    • CDDL
  • pass ./project_precommit_check script run

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

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jan 20, 2024

@0xTim @gwynne I want an approval from you as well 🙂 (Just a 👍 can be enough)

@0xTim
Copy link

0xTim commented Jan 22, 2024

LGTM

@justice-adams-apple
Copy link
Contributor

@swift-ci test

@justice-adams-apple
Copy link
Contributor

👋 @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
Copy link
Contributor Author

MahdiBM commented Jan 24, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

MahdiBM commented Jan 29, 2024

@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
Copy link
Contributor Author

MahdiBM commented Jan 30, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

@swift-ci test

@justice-adams-apple justice-adams-apple merged commit 9421047 into apple:main Jan 30, 2024
1 check passed
@MahdiBM MahdiBM deleted the patch-1 branch January 30, 2024 23:34
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.

None yet

3 participants