SwiftPackageIndex / SwiftPackageIndex-Server

The Swift Package Index is the place to find Swift packages!
https://swiftpackageindex.com
Apache License 2.0
543 stars 43 forks source link

Differentiate reasons for build failures, specifically adding a "Swift tools version" reason #3383

Open daveverwer opened 1 week ago

daveverwer commented 1 week ago

First discussed in https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/discussions/3381

Then, skipping builds based on tools version. The interesting thing about https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/discussions/2294, https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/2125, and https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/727 is that we didn't consider adding a different status for those builds that we skip, and so there was no benefit other than to save us some CPU time in our build infrastructure. What might make the difference is if we consider a "tools version failure" a different kind of "failure" to a regular build error. We already extract and store tools version in our package version metadata, so it wouldn't be too hard to insert a tools version failure record before triggering a build.

We could also limit the impact of the changes by making the new "tools version failure" status as associated value enumeration inside our failure state rather than a completely new state, creating "reason for failure" metadata. We could still treat as a failure everywhere we currently do, but that could be displayed differently on the build detail page.

The more I think about this, the more I think we should do it. Scoping this to being a failure reason rather than a full new state for a build to be in should make the impact of the change much simpler and it would be a step forward for our build reporting, as well as giving us the minor speed increase we'd get from not round-tripping to the build infrastructure.

finestructure commented 1 week ago

I think adding a build with status unsupported is a great idea. The reason I abandoned not triggering builds that have outdated tools versions was that the trigger query would get way too complicated if we did that.

However, we can just insert a build record during analysis when we first discover a new version. That way we don't need to touch the trigger query at all. It'll just not select those builds to begin with.

In terms of how to record it, I think we should aim for giving it a proper status like unsupported. Given it's an enum we'll see where it's being used and where we might face trouble.

But by giving it a new case we'll have an easier time extending the result representation in the build details page where we can then easily show those builds as unsupported instead of failed.

This would also allow us to perhaps extend this to platforms in the future. We've had a couple of inquiries how to mark a platform as unsupported and so far the recommendation was to add an #if ... #error .... With this mechanism in place, we could add a new key to .spi.yml where authors can declare a platform unsupported and we skip all its builds in exactly the same way as we would for tools-version.

daveverwer commented 1 week ago

In terms of how to record it, I think we should aim for giving it a proper status like unsupported. Given it's an enum we'll see where it's being used and where we might face trouble.

My thought was that this might make our aggregation up to the matrix more complex, but it works either way.

adincebic commented 1 week ago

@daveverwer I should have some free time starting Sunday and would like to tackle this.

daveverwer commented 1 week ago

@daveverwer I should have some free time starting Sunday and would like to tackle this.

Thank you for the offer of help! We are always happy to accept contributions. This one might be a little tricky to do as a first issue, but let's chat about it. If you use Discord, join us on our server and we can chat about it.