feat: abstract interactions with archive/store by upils · Pull Request #311 · canonical/chisel

upils · 2026-07-15T13:39:56Z

Have you signed the CLA?

The slicer previously resolved packages to archives directly (selectPkgArchives
returning map[string]archive.Archive), which cannot represent packages
distributed from stores. This change introduces a Fetcher interface in the slicer
that abstracts over where a package comes from: resolveFetchers binds the
source-specific arguments at resolution time, so Run calls a uniform
zero-argument Fetch() without knowing whether the package is a deb from an
archive or a bin from a store. Fetch returns the manifestutil.PackageInfo
interface, letting each fetcher produce its own metadata type (the store will use
revisions and SHA-384 digests). Store packages resolve successfully but fail with
a clear "not implemented" error at fetch time, so the upcoming store support
lands entirely inside binFetcher without touching the slicer again. Resolution
logic is covered by dedicated tests, exported to the test package via export_test.go.

Signed-off-by: Paul Mars <[email protected]>

upils · 2026-07-15T13:47:35Z

+// the package comes from an archive or a store.
+type pkgSource struct {
+	arch  string
+	fetch func() (io.ReadSeekCloser, *archive.PackageInfo, error)


[Note to reviewer]: If we adopt this approach, PackageInfo should likely be extracted to its own package as it will not be specific to the archive anymore and should also be usable by the future store package. This is slightly tangential to changes of this PR so I deffered this change for now.

I'm not completely sure it makes sense to have PackageInfo itself to be extracted to a different package, because by the end of the day an apt package archive has package info data, and this needs to be concretely somewhere. The alternative is that we create a common ground type, but these often end up very messy because the become the union of all needs of all backends. It seems better to extend the approach that we are already pursuing here: interfaces that encapsulate only what actually needs to be common across them.

I'll take this comment to continue into a more general direction: this PR as a whole is playing with the right ideas and going into the right direction, but it's not yet nailing a proper encapsulation and abstraction approach to implement what we must. It doesn't make sense for the slider to be the one defining how to fetch packages and how to fetch stores, by injecting a lambda with custom code for either. The slider is precisely the thing that shouldn't care about the detais, and should instead be calling out in a single way towards multiple implementations.

So again, you're probably in the right ground, but needs some tuning still.

The alternative is that we create a common ground type

This is what I experimented on in #316. Today I think it looks reasonable and not too messy thanks to the introduction of the Digest/DigestKind fields. We end up with a struct that is partially empty in the two existing cases (package from the archive or bin from the store) and the only place consuming it, manifestutil knows how to write/read it from/to a manifest.

I also tried using an interface and keeping 2 separate concrete types in #320 (the one for bins will be added in the PR fetching bins from the store). It seems more consistent with the overall approach. The main drawback is that as this time the added Pkg... methods seems a bit overkill, but this is an investment.

I'll take this comment to continue into a more general direction: this PR as a whole is playing with the right ideas and going into the right direction, but it's not yet nailing a proper encapsulation and abstraction approach to implement what we must. It doesn't make sense for the slider to be the one defining how to fetch packages and how to fetch stores, by injecting a lambda with custom code for either. The slider is precisely the thing that shouldn't care about the detais, and should instead be calling out in a single way towards multiple implementations.

I have reworked the approach following your suggestion. It can be further adapted if we proceed with #320.

Signed-off-by: Paul Mars <[email protected]>

niemeyer · 2026-08-07T18:20:24Z

+// the package comes from an archive or a store.
+type pkgSource struct {
+	arch  string
+	fetch func() (io.ReadSeekCloser, *archive.PackageInfo, error)


I'm not completely sure it makes sense to have PackageInfo itself to be extracted to a different package, because by the end of the day an apt package archive has package info data, and this needs to be concretely somewhere. The alternative is that we create a common ground type, but these often end up very messy because the become the union of all needs of all backends. It seems better to extend the approach that we are already pursuing here: interfaces that encapsulate only what actually needs to be common across them.

I'll take this comment to continue into a more general direction: this PR as a whole is playing with the right ideas and going into the right direction, but it's not yet nailing a proper encapsulation and abstraction approach to implement what we must. It doesn't make sense for the slider to be the one defining how to fetch packages and how to fetch stores, by injecting a lambda with custom code for either. The slider is precisely the thing that shouldn't care about the detais, and should instead be calling out in a single way towards multiple implementations.

So again, you're probably in the right ground, but needs some tuning still.

Add a source package with a Source interface to abstract over the archive and store interface. This is needed because fetching from the store requires the architecture, track and risk.

upils · 2026-08-24T06:27:45Z

+// Source is a resolved package source, abstracting over archives and stores.
+type Source interface {
+	Arch() string
+	Fetch() (io.ReadSeekCloser, *archive.PackageInfo, error)


[Note to reviewer]: If the approach of #320 is adopted, then this method will change to Fetch() (io.ReadSeekCloser, manifestutil.PackageInfo, error)

That's mostly okay, but let's talk about whether manifestutil is the right place for this.

Yes, this place is questionable. See my other note #311 (comment) for the options considered.

niemeyer

This seems close, but I think it needs a once over with some additional care for the concepts being represented. Some hints below, but please consider the overall design a bit further.

niemeyer · 2026-08-31T15:01:30Z

@@ -0,0 +1,110 @@
+package source


This doesn't seem like a great name or place for this. We have several concepts of "source", and this doesn't seem to match cleanly with people's expectations of what they'd find here. It's also weird that this is completely detached from other packages that actually makes this relevant.

The revised approach addresses these concerns:

The Fetcher concept better express what we want to achieve and what the interface offers.

This implementation is now located in the slicer package, but in a dedicated file to still express this is a related but adjacent concern.

If we ever need to expose more common behavior (for example an interface to give info on packages gathered from the archive/store) another interface (ex. Explainer) could be defined.

A case could be made to move this fetcher.go out of the slicer package as in the future cmd_debug_check_release_archive.go could consume it. But at this point this is hypothetical and it seems too weak of a reason to justify a dedicated package for now.

Note on the "fetch" term: in setup we already have a FetchRelease (accepting FetchOptions) but this is a different concern, clearly stated in the name. So I think the risk of confusion is limited. If we want to be completely safe from that, "retrieve" and "Retriever" (for the interface) might be a suitable alternative.

niemeyer · 2026-08-31T15:02:43Z

+}
+
+// archiveSource adapts an archive.Archive to the Source interface for a
+// specific package.


That sounds unusual as well. Adapt an "archive" to a "package source"? It doesn't feel natural to transform one into the other.

Yes it sounded wrong. In the revised approach this adapter is now a debFetcher, and the other one a binFetcher. Their purpose and what they offer is clearer that way.

Signed-off-by: Paul Mars <[email protected]>

upils · 2026-09-01T13:02:00Z

+	"slices"
+
+	"github.com/canonical/chisel/internal/archive"
+	"github.com/canonical/chisel/internal/manifestutil"


[Note to reviewer]: This is a smell that having PackageInfo in manifestutil shows its limit, as already discussed.

I considered moving it to the slicer pkg but that would be wrong because manifestutil (a leaf package) would need to import it.
I also considered creating a standalone package with fetch.go and PackageInfo but that would couple manifestutil with this fetch package and it looks wrong too.

So the next best solution looks to be a small pkginfo leaf package, holding the interface. Let me know what you think about it and I will proceed with it in a follow-up PR (to not mix refactor and feature in this one) if we agree on a refactor.

Signed-off-by: Paul Mars <[email protected]>

upils · 2026-09-03T11:32:11Z

+// package slices file. For packages from a store it selects the store
+// named in the package slices file. It returns a map of Fetcher indexed
+// by package names.
+func selectPkgFetchers(archives map[string]archive.Archive, selection *setup.Selection) (map[string]Fetcher, error) {


[Note to reviewer]: I think it makes sense to have this function here, but it is a refactor (and rename) of selectPkgArchives so moving it right now make it a bit difficult to see the diff. The actual changes in this function are very local but if it is too hard to read I can move it back to slicer.go in this PR and do the move to this file in a follow-up.

upils added 3 commits July 10, 2026 15:58

feat: prepare interface for store

c5d397c

Signed-off-by: Paul Mars <[email protected]>

fix: error fetching from the store

f79521e

Signed-off-by: Paul Mars <[email protected]>

fix: cleaning

a2e22f2

Signed-off-by: Paul Mars <[email protected]>

upils added the Priority Look at me first label Jul 15, 2026

upils changed the title ~~feat: abstract interactions on archive/store~~ feat: abstract interactions with archive/store Jul 15, 2026

upils commented Jul 15, 2026

View reviewed changes

Comment thread internal/slicer/slicer.go Outdated

upils commented Jul 15, 2026

View reviewed changes

upils added 2 commits July 15, 2026 15:52

feat: remove arch setting until store implementation

2eb85d0

Signed-off-by: Paul Mars <[email protected]>

test: refine

3004a6e

Signed-off-by: Paul Mars <[email protected]>

upils mentioned this pull request Jul 15, 2026

feat: common interface for archive/store upils/chisel#20

Closed

1 task

niemeyer requested changes Aug 7, 2026

View reviewed changes

upils added 2 commits August 18, 2026 11:54

refactor: adopt alternative approach

ed0e070

Add a source package with a Source interface to abstract over the archive and store interface. This is needed because fetching from the store requires the architecture, track and risk.

style: remove unused track and risk

268501e

upils commented Aug 24, 2026

View reviewed changes

niemeyer requested changes Aug 31, 2026

View reviewed changes

upils added 3 commits September 1, 2026 14:35

fix: rework fetcher interface

107103d

Signed-off-by: Paul Mars <[email protected]>

Merge branch 'main' into slicer-source-interface

1249e7b

refactor: use the manifestutil.PackageInfo interface

4cfed24

Signed-off-by: Paul Mars <[email protected]>

upils commented Sep 1, 2026

View reviewed changes

upils added 5 commits September 2, 2026 09:16

test: remove redundant tests

7b124b3

Signed-off-by: Paul Mars <[email protected]>

style: renaming and tests simplification

076ff37

fix: revert unintended changes

eaddfd9

docs: use package slices file term

bb39d06

test: revert unrelated changes

b930f97

upils commented Sep 3, 2026

View reviewed changes

Uh oh!

Conversation

upils commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

upils Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

upils Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

upils commented Jul 15, 2026 •

edited

Loading

upils Jul 15, 2026 •

edited

Loading

upils Sep 2, 2026 •

edited

Loading