Skip to content
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.

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

feat(24.04): add bzip2 slice #337

Open
wants to merge 1 commit into
base: ubuntu-24.04
Choose a base branch
from

Conversation

Meulengracht
Copy link
Member

Proposed changes

Add the bzip2 package

Related issues/PRs

Forward porting

Can be proposed for 24.10 if approved

Checklist

@Meulengracht Meulengracht changed the title slices: add bzip2 slice feat(24.04): add bzip2 slice Sep 4, 2024
Copy link

github-actions bot commented Sep 4, 2024

Diff of dependencies:
None found.


Copy link

@clay-lake clay-lake left a comment

Choose a reason for hiding this comment

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

Thanks this is a good package to add, although I think the bins slice needs to be split.

essential:
- libbz2-1.0_libs
- libc6_libs
contents:

Choose a reason for hiding this comment

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

Some of these bins are actually scripts and probably need dash_bins and one or more base-files slices. Could we separate the executables like this xz-utils PR? https://github.com/canonical/chisel-releases/pull/338/files

/usr/bin/bzdiff:       POSIX shell script, ASCII text executable
/usr/bin/bzexe:        POSIX shell script, ASCII text executable
/usr/bin/bzgrep:       POSIX shell script, ASCII text executable
/usr/bin/bzmore:       POSIX shell script, ASCII text executable

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we want to introduce dash as a dependency? What if people want to use bash?

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked questions in your PR so we can clarify this - I'm not sure I completely agree with this

Choose a reason for hiding this comment

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

No problem. btw that draft PR just contains some examples for another PR I was assisting, it probably wont get merged. Most of the work belongs to endersonmaia, Maybe you want to move some of the comments to -> #338

Choose a reason for hiding this comment

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

dash is the default system shell on debian based systems. It is normally linked to /bin/sh. As far as I know, users can still install and execute bash form bash_bins

Here is an example from a relatively fresh multipass instance

ubuntu@noble:~$ head -n 10 /usr/bin/bzmore
#!/bin/sh

# Bzmore wrapped for bzip2, 
# adapted from zmore by Philippe Troin <[email protected]> for Debian GNU/Linux.

PATH="/usr/bin:$PATH"; export PATH

prog=`echo $0 | sed 's|.*/||'`
case "$prog" in
	*less)	more=less	;;
ubuntu@noble:~$ ll /bin/sh
lrwxrwxrwx 1 root root 4 Mar 31 10:47 /bin/sh -> dash*

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's the default shell in ubuntu, but I question whether or not it should be a forced dependency when alternatives exist and it's not a dependency for the official package. Some dependencies make sense to introduce as they are actually required to run and it's pretty binary (i.e coreutils vs not coreutils), but the choice of shell interpreter, dash is not actually a required choice.

I'm not saying I'm right here, but I just want to explain my opinion about why I think it's shouldn't be, if you and your team feel strongly otherwise I'll let it be

Copy link

@clay-lake clay-lake Sep 6, 2024

Choose a reason for hiding this comment

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

I question whether or not it should be a forced dependency when alternatives exist

I agree, and that is a good point. Our team had a short discussion about this recently. What if someone wanted a busybox based image? I think we need a larger discussion on this, but its probably outside the scope of this PR. Especially since some other slices use dash for the same reason.

Not to argue against my first point, but for the time being I think we should be consistent with expectations of a Ubuntu 24.04 distribution. This more reliable since it is widely used as a default shell, and it is what an end user probably expects to be there.

I'd like to hear more voices on this, I've only been on the project for a few months. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@rebornplusplus @cjdcordeiro I belive we need some more input here

Choose a reason for hiding this comment

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

I had a discussion with @cjdcordeiro yesterday on this. We'll get the user to link in the shell after chiseling. The current PR should be fine then. Apologies for the confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Y I think this is something that may get some peace of mind once we can also handle conflicts. But for now, I can actually suggest one thing, if needed (I won't stop the PR because of this): you can create shell-specific slices. If really needed, one can have bins-dash, etc. to create options.

I'm not sure this is the ideal UX, but it surely is possible and avoids any post-inst hooks.

@clay-lake clay-lake mentioned this pull request Sep 9, 2024
3 tasks
essential:
- libbz2-1.0_libs
- libc6_libs
contents:

Choose a reason for hiding this comment

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

I had a discussion with @cjdcordeiro yesterday on this. We'll get the user to link in the shell after chiseling. The current PR should be fine then. Apologies for the confusion.

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

lgtm but we need to test all binaries and scripts, just to make sure they're properly loaded.

And as alluded by the above discussion, if there are scripts that assume a post-inst operation, then at least leave a comment in the YAML

@linostar
Copy link
Collaborator

Hey @Meulengracht
Can you add simple tests for the rest of the bzip binaries/shell scripts, in addition to the comment regarding manuallylinking to /bin/sh?

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.

4 participants