Skip to content
This repository was archived by the owner on Oct 2, 2024. It is now read-only.

new script ch-convert #698

Merged
merged 29 commits into from
Oct 29, 2021
Merged

new script ch-convert #698

merged 29 commits into from
Oct 29, 2021

Conversation

reidpr
Copy link
Collaborator

@reidpr reidpr commented Mar 17, 2020

This PR implements a new script ch-convert that replaces the following scripts, which are now deprecated:

  • ch-build2dir
  • ch-builder2squash
  • ch-builder2tar
  • ch-dir2squash
  • ch-pull2dir
  • ch-pull2tar
  • ch-tar2dir

Relevant to #484.

@reidpr reidpr requested review from rstyd and shanegoff March 17, 2020 23:04
Copy link
Contributor

@shanegoff shanegoff left a comment

Choose a reason for hiding this comment

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

I think everything looks good here.

@reidpr
Copy link
Collaborator Author

reidpr commented Apr 17, 2020

Need to take care with unsquashfs(1) options to avoid:

Parallel unsquashfs: Using 64 processors
20198 inodes (23645 blocks) to write
 
 
write_xattr: could not write xattr security.selinux for file /var/tmp/ior-openmpi-at/bin because you're not superuser!
 
write_xattr: to avoid this error message, either specify -user-xattrs, -no-xattrs, or run as superuser!
 
Further error messages of this type are suppressed!
[=============================================================================================- ] 23644/23645  99%
 
created 17960 files
created 2059 directories
created 2020 symlinks
created 0 devices
created 0 fifos

Thanks for catching this @atorrez and @Brofessional!

@reidpr reidpr marked this pull request as draft March 15, 2021 20:02
@reidpr reidpr closed this Apr 7, 2021
@reidpr reidpr reopened this Aug 13, 2021
Copy link
Contributor

@shanegoff shanegoff left a comment

Choose a reason for hiding this comment

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

I like the new approach. I'm sure I'll mess up the formats every time trying to do implicit so I'm happy about the explicit options, it looks a little more the way I'd be used to exporting images.

I am totally unreasonably opposed to adding other output extensions for squashfs. No strong reason, it just makes output extensions more complicated.

down with .squash and .squashfs long live .sqfs

Copy link
Contributor

@j-ogas j-ogas left a comment

Choose a reason for hiding this comment

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

Looks good. Are we still planning to add the conveniece subcommand ch-image convert? If so we should also add the documentation draft for that here as well.

@reidpr
Copy link
Collaborator Author

reidpr commented Sep 2, 2021

Are we still planning to add the conveniece subcommand ch-image convert?

Yes, though I think it would be three subcommands: dir, tar, squash. I was thinking that would be a separate PR though. What's your argument it should be included here?

@j-ogas
Copy link
Contributor

j-ogas commented Sep 2, 2021

Yes, though I think it would be three subcommands: dir, tar, squash. I was thinking that would be a separate PR though. What's your argument it should be included here?

Ah, three explicit subcommands sounds like a better approach. In that case, yes, a separate PR seems appropriate.

@reidpr reidpr requested review from shanegoff and j-ogas October 20, 2021 20:04
@reidpr reidpr marked this pull request as ready for review October 20, 2021 20:04
Copy link
Contributor

@shanegoff shanegoff left a comment

Choose a reason for hiding this comment

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

Overall I think everything is working as intended. Only a few small things here and there that I thought were maybe confusing in the documentations.

@reidpr
Copy link
Collaborator Author

reidpr commented Oct 21, 2021

More to-do:

@reidpr reidpr requested a review from shanegoff October 21, 2021 21:57
Copy link
Contributor

@shanegoff shanegoff left a comment

Choose a reason for hiding this comment

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

This looks great. I look forward to the one wrapper to rule them all.

Copy link
Contributor

@j-ogas j-ogas left a comment

Choose a reason for hiding this comment

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

LGTM!

@reidpr reidpr merged commit 5a71aa1 into master Oct 29, 2021
@reidpr reidpr deleted the goff-1 branch October 29, 2021 16:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants