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’s archive-specific package resolution could not accommodate packages
fetched from stores. This refactor establishes a provider-neutral fetch boundary
while preserving archive selection behavior.

It keeps the target architecture used for slicing separate from the fetched package
architecture recorded in the manifest.

Manifests retain real package names, with an optional alias linking packages to
prefixed slice names. Store fetching remains explicitly unimplemented; its
integration and store-specific manifest fields will follow separately.

Signed-off-by: Paul Mars <paul.mars@canonical.com>

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 <paul.mars@canonical.com>

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 <paul.mars@canonical.com>

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 <paul.mars@canonical.com>

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.

Thanks for the heads up. Yes, let's please move it back so we can discuss the delta for now. We can reorganize as a follow up.

niemeyer

It's walking to a good direction, but not there yet. Details below.

niemeyer · 2026-09-28T16:29:57Z

+
+type Fetcher interface {
+	Arch() string
+	Fetch() (io.ReadSeekCloser, manifestutil.PackageInfo, error)


As previously discussed, this doesn't seem proper: the manifest is a file that is supposed to be informative, contain the common high-level details for the package, that actually goes into the manifest. This, instead, is information we get from a remote package provider. These things don't need to match (and likely won't). Somehow, we will certainly be able to obtain manifest data out of the package, and maybe have themselves directly implementing such an interface.

You've already felt that issue without actually putting your finger on the pain, because you are struggling with import relationships. The reason you struggle is because the abstractions aren't quite right yet.

Let's give this a shot: Fetcher is actually a RemotePackage, and we can get a RemotePackageInfo from it.

Now, there's still something else strange here: we have Arch on the Fetcher, which doesn't make much sense, and it also means we have two places for this, one when we fetch, one before. So there's still more to be polished around this issue besides these two names I suggest above.

As previously discussed, this doesn't seem proper: the manifest is a file that is supposed to be informative, contain the common high-level details for the package, that actually goes into the manifest. This, instead, is information we get from a remote package provider. These things don't need to match (and likely won't).

I have now more clearly separated them, let me know if this is what you meant or not.

Now, there's still something else strange here: we have Arch on the Fetcher, which doesn't make much sense, and it also means we have two places for this, one when we fetch, one before. So there's still more to be polished around this issue besides these two names I suggest above.

Good catch, see my comment on the addition of the Arch field in Selection.

niemeyer · 2026-09-28T16:32:10Z

+// 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) {


Thanks for the heads up. Yes, let's please move it back so we can discuss the delta for now. We can reorganize as a follow up.

niemeyer

👆

upils · 2026-10-01T07:43:47Z

+)
+
+// RemotePackageInfo describes a package as reported by its provider.
+type RemotePackageInfo struct {


[Note to reviewer]: Following your suggestion, the archive.PackageInfo (and the future store.PackageInfo) does not satisfy the interface defined by manifestutil anymore, but RemotePackageInfo does. The Fetch method below does the fetching and converts the received PackageInfo.

upils · 2026-10-01T07:53:50Z

 type Selection struct {
 	Release *Release
 	Slices  []*Slice
+	Arch    string


[Note to reviewer]: So far in Run() we were using the Arch stored in the options given to archives. Trying to apply this logic led to expose a Arch() method on the RemotePackage interface. This in turn led to confusion as the arch value reported for a package is not necessarily the same as the one for the whole selection (a deb package can report "all"), and led to exposing these 2 values at the same time.

When cutting, the arch value we want is not package-specific, it is a property of the whole selection.

So it seems cleaner to store the arch used when doing the selection and use it in Run().

upils added 3 commits July 10, 2026 15:58

feat: prepare interface for store

c5d397c

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: error fetching from the store

f79521e

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: cleaning

a2e22f2

Signed-off-by: Paul Mars <paul.mars@canonical.com>

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 <paul.mars@canonical.com>

test: refine

3004a6e

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils mentioned this pull request Jul 15, 2026

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

Closed

1 task done

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 <paul.mars@canonical.com>

Merge branch 'main' into slicer-source-interface

1249e7b

refactor: use the manifestutil.PackageInfo interface

4cfed24

Signed-off-by: Paul Mars <paul.mars@canonical.com>

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 <paul.mars@canonical.com>

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

docs: clean useless comment

8c24bda

niemeyer reviewed Sep 28, 2026

View reviewed changes

niemeyer requested changes Sep 28, 2026

View reviewed changes

fix: apply review comments

a13c637

upils commented Oct 1, 2026

View reviewed changes

upils added 4 commits October 1, 2026 14:12

fix: revert unwanted changes

3655843

fix: move out fix for package naming collision

a81c9a7

fix: simplify arch check in Run

b932d17

docs: fix comment

1a997ea

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!

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!

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 Oct 1, 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!

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

upils Oct 1, 2026 •

edited

Loading