feat: support slice definition files for bins#15
Conversation
letFunny
left a comment
There was a problem hiding this comment.
This looks very very good! I just have a few minor comments (as usual :P)
| } | ||
| return pkgArchive, nil | ||
|
|
||
| // Store packages do not have an archive to derive the architecture from. |
There was a problem hiding this comment.
This looks a little bit like a hack. If the architecture is the same for all packages and bins because it is only set by --arch CLI argument, then either we don't need it in the source information because it is available elsewhere and it is not necessary to carry it in each one of them; or, alternatively, we can be more conservative by passing the arch from cmd_cut all the way here and setting all the arch(s) for packages and bins using that one.
I understand this requires a refactor that would be only tangentially related to the PR and this might get some push-back so there is a reason not to do it.
There was a problem hiding this comment.
I confirm this is definitely a hack until I add the store as a package source. When doing so, the "opening" of the store in cmd_cut should fill the selected arch, which can then be used here. I will at least rework this comment to clarify that this logic is here just to make sure the rest can proceed properly for now.
There was a problem hiding this comment.
Isn't the arch set globally by the CLI arg? Then I guess my point is that all the logic should not look at the arch separately. Not saying this is hacky, I am saying it can be simplified and both archives and stores could get the arch from a simple arg to the function that opens them. We might reject the proposal to keep it more generic but at the moment the dance is:
- cmd_cut gets a CLI arch.
- cmd_cut opens all the archives with that CLI arch.
- slicer looks in the archives for each package and stores the archive arch in the package.
If in the future we want to pull packages from two different archs then it makes sense, if not my point is that this is too flexible for no valid use-case.
There was a problem hiding this comment.
I am saying it can be simplified and both archives and stores could get the arch from a simple arg to the function that opens them
I plan to do exactly that in cmd_cut.go but it does not really change the fact that each pkgSourceInfo is then later storing the arch value.
If in the future we want to pull packages from two different archs then it makes sense, if not my point is that this is too flexible for no valid use-case.
I agree and so far I could not see a valid use case where Chisel would build a rootfs with 2 different architectures at the same time. I suspect surfacing the arch in pkgSourceInfo is to avoid going through the archive field whenever we need the arch value later in the process.
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
letFunny
left a comment
There was a problem hiding this comment.
Paul I think this is ready! It looks very good tbh. Given the constraints and the desire to minimize refactors I don't have any more comments apart from the last set of questions :P
| id: ` + testKey.ID + ` | ||
| armor: |` + "\n" + PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") | ||
|
|
||
| var DefaultChiselYamlWithStores = strings.ReplaceAll(DefaultChiselYaml, "format: v1", "format: v3") + ` |
There was a problem hiding this comment.
Nit: Up to personal opinion but I prefer to inline it just to see the whole thing if I ever need to debug it. And also to prevent unrelated changes in one variable affecting the other.
There was a problem hiding this comment.
As DefaultChiselYaml is just above and the replace on it is minimal I thought it was still readable while easy to maintain. I wanted to avoid having these 2 default YAMLs to go out of sync and us spending time diagnosing why some tests fail and not others. I will leave it like that for now and see what G thinks of it.
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Signed-off-by: Paul Mars <paul.mars@canonical.com>
| if pkg.Store != "" { | ||
| pkgSources[pkg.Name] = &pkgSourceInfo{ | ||
| // TODO: Fill with the live store handle when store support is implemented. | ||
| kind: sourceStore, |
There was a problem hiding this comment.
[Note to reviewer]: Next PR will add the fetching from the store, so open stores will be accessible in this function. It can then be used to fill the arch and the associated store.
Idea:
func resolvePkgSources(archives map[string]archive.Archive, stores map[string]store.Store, selection *setup.Selection) (map[string]*pkgSourceInfo, error) {
[...]
if pkg.Store != "" {
storeHandle := stores[pkg.Store]
if storeHandle == nil {
return nil, fmt.Errorf("internal error: no store handle for store %q", pkg.Store)
}
pkgSources[pkg.Name] = &pkgSourceInfo{
arch: storeHandle.Options().Arch,
kind: sourceStore,
store: storeHandle,
pkg: pkg,
}
continue
}
Introduce the actual client that talks to the bin store, and wires it into the cut pipeline.
Store package extraction is still not performed here — this PR provides the store client
and the plumbing to open stores declared by a release.