Skip to content

feat: abstract interactions with archive/store #311

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.

Sign up for GitHub

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

Open
upils wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: abstract interactions with archive/store #311

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c5d397c
feat: prepare interface for store
upils Jul 9, 2026
f79521e
fix: error fetching from the store
upils Jul 10, 2026
a2e22f2
fix: cleaning
upils Jul 10, 2026
2eb85d0
feat: remove arch setting until store implementation
upils Jul 15, 2026
3004a6e
test: refine
upils Jul 15, 2026
ed0e070
refactor: adopt alternative approach
upils Aug 18, 2026
268501e
style: remove unused track and risk
upils Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 36 additions & 14 deletions internal/slicer/slicer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ type contentChecker struct {
knownPaths map[string]pathData
}

// pkgSource holds the resolved source for a package: its architecture and a
// fetch function returning the package reader and metadata. The fetch
// function is bound at resolution time, so callers are agnostic to whether
// the package comes from an archive or a store.
type pkgSource struct {

@upils upils Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Note to reviewer]: This struct could be extracted to a dedicated package and exported since in the future this abstraction might be useful in other packages, such as cmd_debug_check_release_archives.go.

arch string
fetch func() (io.ReadSeekCloser, *archive.PackageInfo, error)

@upils upils Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@upils upils Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #316

@niemeyer niemeyer Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@upils upils Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (cc *contentChecker) checkMutable(path string) error {
if !cc.knownPaths[path].mutable {
return fmt.Errorf("cannot write file which is not mutable: %s", path)
Expand Down Expand Up @@ -90,7 +99,7 @@ func Run(options *RunOptions) error {
targetDir = filepath.Join(dir, targetDir)
}

pkgArchive, err := selectPkgArchives(options.Archives, options.Selection)
pkgSources, err := resolvePkgSources(options.Archives, options.Selection)
if err != nil {
return err
}
Expand All @@ -108,7 +117,7 @@ func Run(options *RunOptions) error {
extractPackage = make(map[string][]tarball.ExtractInfo)
extract[slice.Package] = extractPackage
}
arch := pkgArchive[slice.Package].Options().Arch
arch := pkgSources[slice.Package].arch
for targetPath, pathInfo := range slice.Contents {
if targetPath == "" {
continue
Expand Down Expand Up @@ -153,7 +162,7 @@ func Run(options *RunOptions) error {
continue
}
pkg := options.Selection.Release.Packages[slice.Package]
reader, info, err := pkgArchive[slice.Package].Fetch(pkg.RealName)
reader, info, err := pkgSources[pkg.Name].fetch()
if err != nil {
return err
}
Expand Down Expand Up @@ -270,7 +279,7 @@ func Run(options *RunOptions) error {
// them to the appropriate slices.
relPaths := map[string][]*setup.Slice{}
for _, slice := range options.Selection.Slices {
arch := pkgArchive[slice.Package].Options().Arch
arch := pkgSources[slice.Package].arch
for relPath, pathInfo := range slice.Contents {
if len(pathInfo.Arch) > 0 && !slices.Contains(pathInfo.Arch, arch) {
continue
Expand Down Expand Up @@ -490,10 +499,13 @@ func createFile(targetDir, relPath string, pathInfo setup.PathInfo) (*fsutil.Ent
})
}

// selectPkgArchives selects the highest priority archive containing the package
// unless a particular archive is pinned within the slice definition file. It
// returns a map of archives indexed by package names.
func selectPkgArchives(archives map[string]archive.Archive, selection *setup.Selection) (map[string]archive.Archive, error) {
// resolvePkgSources determines the source for each package in the selection.
// For archive packages it selects the highest priority archive containing the
// package unless a particular archive is pinned within the slice definition
// file. For store packages it records a fetch function that returns an error
// until store support is implemented. It returns a map of pkgSource indexed by
// package names.
func resolvePkgSources(archives map[string]archive.Archive, selection *setup.Selection) (map[string]pkgSource, error) {
sortedArchives := make([]*setup.Archive, 0, len(selection.Release.Archives))
for _, archive := range selection.Release.Archives {
if archive.Priority < 0 {
Expand All @@ -507,15 +519,20 @@ func selectPkgArchives(archives map[string]archive.Archive, selection *setup.Sel
return b.Priority - a.Priority
})

pkgArchive := make(map[string]archive.Archive)
pkgSources := make(map[string]pkgSource)
for _, s := range selection.Slices {
if _, ok := pkgArchive[s.Package]; ok {
if _, ok := pkgSources[s.Package]; ok {
continue
}
pkg := selection.Release.Packages[s.Package]

if pkg.Store != "" {
return nil, fmt.Errorf("cannot fetch package %q from store %q: not implemented", pkg.Name, pkg.Store)
pkgSources[pkg.Name] = pkgSource{
// TODO: set the arch when implementing fetching from the store.
fetch: func() (io.ReadSeekCloser, *archive.PackageInfo, error) {
return nil, nil, fmt.Errorf("cannot fetch package %q from store %q: not implemented", pkg.Name, pkg.Store)
},
}
continue
}

var candidates []*setup.Archive
Expand All @@ -538,7 +555,12 @@ func selectPkgArchives(archives map[string]archive.Archive, selection *setup.Sel
if chosen == nil {
return nil, fmt.Errorf("cannot find package %q in archive(s)", pkg.RealName)
}
pkgArchive[pkg.Name] = chosen
pkgSources[pkg.Name] = pkgSource{
arch: chosen.Options().Arch,
fetch: func() (io.ReadSeekCloser, *archive.PackageInfo, error) {
return chosen.Fetch(pkg.RealName)
},
}
}
return pkgArchive, nil
return pkgSources, nil
}
24 changes: 16 additions & 8 deletions internal/slicer/slicer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1979,21 +1979,29 @@ var slicerTests = []slicerTest{{
"/dir/file": "file 0644 cc55e2ec {test-package_third}",
},
}, {
summary: "Store package is not yet implemented",
slices: []setup.SliceKey{{"bin-curl", "bin"}},
summary: "Store package fetching not yet implemented",
slices: []setup.SliceKey{{"test-package", "myslice"}, {"bin-store-pkg", "myslice"}},
arch: "amd64",
release: map[string]string{
"chisel.yaml": testutil.DefaultChiselYamlWithStores,
"slices/curl.yaml": `
package: curl
"slices/mydir/test-package.yaml": `
package: test-package
slices:
myslice:
contents:
/dir/file:
`,
"slices/mydir/store-pkg.yaml": `
package: store-pkg
store: bin
default-track: latest
default-track: 3.1
slices:
bin:
myslice:
contents:
/usr/bin/curl:
/dir/store-file:
`,
},
error: `cannot fetch package "bin-curl" from store "bin": not implemented`,
error: `cannot fetch package "bin-store-pkg" from store "bin": not implemented`,
}}

func (s *S) TestRun(c *C) {
Expand Down
Loading