Skip to content

feat: support bin packages parsing#15

Draft
upils wants to merge 36 commits into
mainfrom
bin-slices
Draft

feat: support bin packages parsing#15
upils wants to merge 36 commits into
mainfrom
bin-slices

Conversation

@upils

@upils upils commented Jun 18, 2026

Copy link
Copy Markdown
Owner
  • Have you signed the CLA?

upils and others added 30 commits May 27, 2026 16:06
Signed-off-by: Paul Mars <paul.mars@canonical.com>
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>
upils added 5 commits June 17, 2026 08:32
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 letFunny left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks very very good! I just have a few minor comments (as usual :P)

Comment thread internal/setup/setup.go
Comment thread internal/setup/setup.go
Comment thread internal/slicer/slicer.go
}
return pkgArchive, nil

// Store packages do not have an archive to derive the architecture from.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread internal/slicer/slicer.go
Comment thread internal/setup/yaml.go
if details.Kind == "" {
return nil, fmt.Errorf("%s: store %q missing kind field", fileName, storeName)
}
if details.Kind != "bin" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread internal/setup/setup_test.go Outdated
Comment thread internal/setup/setup_test.go
Comment thread internal/setup/setup_test.go
Comment thread internal/setup/yaml.go
Comment thread internal/setup/yaml.go Outdated
Signed-off-by: Paul Mars <paul.mars@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants