Accept debian:upload artifacts in the sbuild task and workflows

Builds on top of !1447 (merged) (with the same annoying coverage problem that I can't reproduce).

It just looks through the debian:upload to see the debian:source-package, and doesn't do anything special with the debian:upload when it comes to uploading.

I'm open to suggestions on what to do about the debian_utils modules. Not great names (I think "utils" is usually a code smell). And having two implementations of the same thing isn't ideal.

Fixes: #590 (closed)

changed the description

added 15 commits

• bc89469a...189cde9b - 6 commits from branch devel
• 75b3c130 - Implement ArtifactPlayground.create_upload()
• 48f6f0b9 - Implement Playground.create_upload_artifact()
• 8a4d590c - The Binary field in a .changes file is not comma-separated
• 8a3ed8c1 - Avoid a coverage bug on bookworm
• b8d51f1e - Factor out a function to locate the debian:source-package
• ad7a2b9a - sbuild task: Accept debian:upload as source
• b4ffb0bb - Reimplement locate_debian_source_artifact from server-side
• 1b8d0def - sbuild workflow: Accept debian:upload as source
• 11ab0998 - Use a debian:source-package in Workflow Tests

Compare with previous version

It feels to me as though this goes to more work than really necessary. A debian:upload artifact should contain a superset of the files that are in the debian:source-package artifact it extends - indeed I think that's generally what the extends relation means - and for the most part the sbuild task and workflow really just care that they can find the .dsc file and the other files that it refers to. In an ideal world, I'd prefer this to just be:

        case (ArtifactCategory.SOURCE_PACKAGE, ArtifactCategory.UPLOAD):
            return input_artifact

Same on the task side - and with a simpler implementation, perhaps a way to avoid the duplication can be found.

Now, there are some problems with this approach. The sbuild task wants to create relations pointing specifically to the debian:source-package artifact, and the sbuild workflow needs access to information from the debian:source-package artifact data, which is why you needed to go to more effort. But other tasks almost certainly have variants of the same problem, involving different categories and relations, and besides I don't really like an API that implies that this is always something you need to do if you want a source package artifact - the two call sites in question only need it because of additional things they do. Could we instead have separate functions with signatures something like this?

def require_artifact_categories(artifact: Artifact, categories: Collection[ArtifactCategory]) -> None:
    ...

def follow_artifact_relation(artifact: Artifact, relation_type: ArtifactRelation.Relations, category: ArtifactCategory) -> Artifact:
    ...

(We don't really need the Architecture: source check - having an extends relation pointing to a source package should be enough.)

An API like this would be much more easily generalizable to other situations; and I think splitting up the "expected category" and "expected relation" checks will make our lives easier down the line.

At this point the interface wouldn't be specific to debian:* categories any more, and it would make sense to move it somewhere more generic. Maybe methods on BaseExternalTask and BaseServerTask would be possibilities?

A debian:upload artifact should contain a superset of the files that are in the debian:source-package artifact it extends

Yeah, I started from that thinking too. But, the relations you mention, and metadata quickly took me to this approach.

I'm thinking that long-term we would want to reduce sbuild to only accept debian:source-package artifacts. If it's useful to feed in a debian:upload, a workflow can do follow the relations, and feed the sbuild task just the source package.

Also, there's the problem that a debian:upload isn't necessarily wrapping a source package.

require_artifact_categories and follow_artifact_relation

That sounds like a good API.

Maybe methods on BaseExternalTask and BaseServerTask would be possibilities?

Yep, with this API, I think that can work.

Although I really want to try to trim down these classes at some point in the future. Lots of functionality in them could be moved out.

Now that I sit down to implement it, I'm less convinced by this approach. These functions would not be a good fit in Base*Task, and I can't import debusine.db in debusine.task.

I think I'll implement something like this just for workflows. Maybe we can get away without ever having this in the sbuild task.

I've been thinking for a while that a lot of artifact handling in tasks and workflows could be more declarative, rather than requiring ad-hoc code in lots of tasks to check that the results of a lookup are as expected. Ideally I'd like a way to annotate a lookup in a task data model with an indication of the artifact categories that the task accepts, and then have the common artifact-fetching machinery in tasks deal with logging errors and failing if the categories are wrong. Perhaps it could even be pushed down to the lookup machinery, similar to the existing expect_type parameter.

You don't have to do this as part of this MR; I imagine it might take a while to settle on the right API, and this MR seems like a reasonable solution to a real problem. But it might inform some module naming decisions, and maybe there are some easy steps that can be taken towards this.

added MR: Needs work label

mentioned in merge request !1516 (merged)

closed