Skip to content

feat: upgrade existing filesystem #264

Open
upils wants to merge 70 commits intocanonical:mainfrom
upils:recut
Open

feat: upgrade existing filesystem #264
upils wants to merge 70 commits intocanonical:mainfrom
upils:recut

Conversation

@upils
Copy link
Collaborator

@upils upils commented Jan 29, 2026

  • Have you signed the CLA?

This commit enables Chisel upgrading an existing rootfs.
It detects the target directory contains the result of a previous execution
to then operate an upgrade of the content. This initial simple implementation
has the limitation that existing content (files, symlinks) is systematically
replaced, even if identical. As a consequences:

  • User modifications on chisel-managed content is overridden.
  • In the context of an OCI image build, this "new" content is identified as
    different and thus the new OCI layer contains duplicated files.

@github-actions
Copy link

github-actions bot commented Jan 29, 2026

Command Mean [s] Min [s] Max [s] Relative
BASE 13.357 ± 0.114 13.191 13.522 1.01 ± 0.01
HEAD 13.280 ± 0.109 13.173 13.509 1.00

@upils upils changed the title feat: update existing filesystem feat: upgrade existing filesystem Feb 2, 2026
@upils upils requested a review from letFunny February 2, 2026 14:35
@upils upils marked this pull request as ready for review February 2, 2026 14:35
Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

First review. I am missing a couple of files but I wanted to publish it as quickly as possible so we have more time to iterate. This is looking great Paul, many things are looking dead simple which is always an extremely good sign. I have written some comments about how to make the PR shorter and how to make it, hopefully, even simpler. Let me know what you think.

I see some of the comments are similar to past discussions we had in upils#1. We spent some effort there, so please make sure all the comments were carried over to this one.

if !os.IsExist(err) {
return err
}
err = os.RemoveAll(dstPath)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[Note to reviewer]: This can silently remove a directory full of user content. This is inline with the simple strategy implemented in this PR but it should likely evolve when a more fine-grained strategy is implemented to "spare" user content.

@upils upils requested a review from letFunny February 25, 2026 14:15
Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

Thanks Paul, this is almost ready. I added a bunch of minor suggestions and a couple important things. Also, please reply to my suggestions from previous reviews some of which are still open and are applicable. Once we close all previous comments and the one in this review I think we are good to go.

optsCopy := *options
installOpts := &optsCopy
installOpts.TargetDir = targetDir
if options.Manifest != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[Note to reviewer]: Should we log a message in that case to inform the user a "recut" will happen?

Deeper, the upgrade function currently logs a message stating an upgrade will happen. I suppose only one of these messages should be logged, since they will always be either present or absent.

@upils upils requested a review from letFunny February 27, 2026 15:32
Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

Thank you Paul for all the back and forth and all the effort put in the PR. I added some nitpicky comments but looking at the whole PR I cannot spot anything big pending; I see some areas where I expect minor discussion, but in general it looks very good. I think it is ready for Gustavo to review, so please resolve the comments in the PR as I don't have permission to do so, and move it to the review queue.

@polarathene
Copy link

  • In the context of an OCI image build, this "new" content is identified as different and thus the new OCI layer contains duplicated files.

FWIW: If using SOURCE_DATE_EPOCH with a supported builder to rewrite timestamps of contents in a layer to the epoch, identical files should no longer contribute additional size IIRC.

@upils upils added the Priority Look at me first label Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority Look at me first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants