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

Add slices for bash and coreutils #61

Merged
merged 9 commits into from
Sep 29, 2023

Conversation

weiiwang01
Copy link

This pull request introduces slice definitions for bash and GNU coreutils, along with some associated libraries.

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Nice! It looks quite good.

I have a few comments below. Let me know what you think of them.

slices/bash.yaml Outdated
- libtinfo6_libs
contents:
/bin/bash:
/bin/sh: { symlink: /bin/bash }
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. this is a tricky one. We should have a symlink to /bin/bash, but perhaps including this in contents might be a bad idea. Because, chisel does not support conflicting paths across slices/packages, yet. See canonical/chisel#50.
So I am worried including /bin/sh might raise this issue. For example, if we were to install dash and then create a symlink in a same way, we will have conflicts.
So, perhaps, it's better to drop this from the contents, at least for now. We could leave a comment/note to remind ourselves that we need to do this manually after installation.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, removed the /bin/sh from contents and added a comment.

/usr/bin/join:
/usr/bin/link:
/usr/bin/logname:
/usr/bin/md5sum:
Copy link
Member

Choose a reason for hiding this comment

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

/usr/bin/md5sum.textutils seems to be missing.

Copy link
Author

Choose a reason for hiding this comment

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

Added, thanks!

/usr/bin/who:
/usr/bin/whoami:
/usr/bin/yes:
/usr/libexec/coreutils/libstdbuf.so:
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in a libs slice.You can then add that slices as an essential in the bins slice.

Copy link
Author

Choose a reason for hiding this comment

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

I'm uncertain if we should place this library in a separate libs section. It appears to be an internal library for coreutils and isn't meant for external use.

Copy link
Member

Choose a reason for hiding this comment

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

It appears to be an internal library for coreutils and isn't meant for external use.

True. But typically we group the libraries (including internal ones) into libs and binaries into bins for better categorization.

Copy link
Author

Choose a reason for hiding this comment

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

Make sense, updated, thanks!

essential:
- libc6_libs
contents:
/lib/*-linux-*/libtinfo.so.6*:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add /usr/lib/*-linux-*/libtic.so.6* as done in https://github.com/canonical/chisel-releases/blob/ubuntu-20.04/slices/libtinfo6.yaml.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, indeed, added, thanks!

/usr/bin/link:
/usr/bin/logname:
/usr/bin/md5sum:
/usr/bin/md5sum.textutils: { symlink: /usr/bin/md5sum }
Copy link
Member

Choose a reason for hiding this comment

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

SInce this already comes as a symlink from the package, you don't need to manually create it.

Suggested change
/usr/bin/md5sum.textutils: { symlink: /usr/bin/md5sum }
/usr/bin/md5sum.textutils:

Copy link
Author

Choose a reason for hiding this comment

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

Updated, thanks!

/usr/bin/who:
/usr/bin/whoami:
/usr/bin/yes:
/usr/libexec/coreutils/libstdbuf.so:
Copy link
Member

Choose a reason for hiding this comment

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

It appears to be an internal library for coreutils and isn't meant for external use.

True. But typically we group the libraries (including internal ones) into libs and binaries into bins for better categorization.

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@rebornplusplus
Copy link
Member

@woky will you please take a look at this PR?

@cjdcordeiro cjdcordeiro self-requested a review September 26, 2023 15:28
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.

Thanks for this! I have a few minor change requests and questions, but in general, it looks good ;)

slices/bash.yaml Show resolved Hide resolved
slices/bash.yaml Show resolved Hide resolved
slices/bash.yaml Show resolved Hide resolved
slices/bash.yaml Show resolved Hide resolved
slices/libattr1.yaml Show resolved Hide resolved
slices/bash.yaml Outdated Show resolved Hide resolved
slices/bash.yaml Show resolved Hide resolved
slices/bash.yaml Outdated Show resolved Hide resolved
slices/bash.yaml Show resolved Hide resolved
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 thanks

slices/bash.yaml Outdated Show resolved Hide resolved
slices/bash.yaml Outdated Show resolved Hide resolved
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.

Thanks again ;)

@cjdcordeiro cjdcordeiro merged commit 6922599 into canonical:ubuntu-22.04 Sep 29, 2023
@cjdcordeiro cjdcordeiro self-assigned this Sep 29, 2023
gregory-schiano pushed a commit to gregory-schiano/chisel-releases that referenced this pull request Jun 22, 2024
cjdcordeiro added a commit to ozanmakes/chisel-releases that referenced this pull request Sep 27, 2024
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.

3 participants