feat: support bin packages parsing#15
Conversation
upils
commented
Jun 18, 2026
- Have you signed the CLA?
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>
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.
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.
| if details.Kind == "" { | ||
| return nil, fmt.Errorf("%s: store %q missing kind field", fileName, storeName) | ||
| } | ||
| if details.Kind != "bin" { |
There was a problem hiding this comment.
This means you will break if you ever introduce other store kinds, not when cutting into the new kind of package but when parsing the release. Meaning, if you are planning to introduce something else you need a hard break, not sure what the plans are at the moment.
There was a problem hiding this comment.
I added this check very recently and thought about this issue. Even if I remove this check, later in the process this version of Chisel will break anyway if anything other than this value is given because it will only supports "bin" stores. Using another value would be like trying to use an archive with an unknown "pro" value, we expect it to fail.
If we ever support another kind of store, we will have to implement the interaction with this store and associate it to the new kind.
So my approach was to fail fast when the kind is unknown.
Do you see another path forward that would avoid a hard break?
There was a problem hiding this comment.
Actually with pro I think we ignore unknown values if I am not mistaken.
The other approach, and I remember we did it for generate is that you fail at runtime:
func Select(...) {
for _, new := range selection.Slices {
for newPath, newInfo := range new.Contents {
// An invalid "generate" value should only throw an error if that
// particular slice is selected. Hence, the check is here.
switch newInfo.Generate {
case GenerateNone, GenerateManifest:
default:
return nil, fmt.Errorf("slice %s has invalid 'generate' for path %s: %q",
new, newPath, newInfo.Generate)
}
}
}
}
So the alternative is to fail when a slice is selected. This means that:
- Existing CI that is configured to use a set of slices will never break when you introduce a new kind in the release.
- Only when changing the selection of packages to some from a different store will Chisel report and error.
Basically: break if you try to use the new feature, avoid breaking if not. The clear disadvantage is that you still need to parse and conflict check the whole release and this adds a bit of complexity.
Signed-off-by: Paul Mars <paul.mars@canonical.com>