feat: use a predictable working dir by upils · Pull Request #5 · upils/chisel

upils · 2026-03-25T10:34:08Z

Have you signed the CLA?

This PR implements two conceptual changes:

When running the slicer, Chisel always creates a .chisel working directory
at the root of the target directory. Existing entry at the same path is remove
to ensure the creation of this directory. This directory is removed at the end
only if it empty. This accomodates future use cases that might leave content
in it on purpose between executions.
When recutting an existing rootfs, a rootfs directory is created under the
.chisel working dir to cut the new content in an isolated place. This
directory is removed at the end of the process.

internal/slicer/slicer_test.go

letFunny

Thanks Paul, this looks very good, only a few minor comments

letFunny · 2026-03-25T11:25:09Z

internal/slicer/slicer.go

-const manifestMode fs.FileMode = 0644
+const (
+	manifestMode fs.FileMode = 0o644
+	rootWorkdir              = ".chisel"


I think the naming is a bit off here. This is not necessarily a working directory, right now we have only one usage but from the conversation the other day it could evolve into doing more things. It also makes it harder later to differentiate between .chisel and .chisel/<workdir_for_recut>.

I tried different names and settled on this one because I thought it was in fact a "working directory". As you can see in the rest of the code, the subdirectory holding the new rootfs is not named "workdir" anymore, to avoid confusion.

metadataDir or stateDir are other potential candidates but I am not really convinced they better encompass what could be stored in this directory. Do you have other ideas?

I have a slight preference for stateDir or something similar yes

internal/slicer/slicer.go

letFunny · 2026-03-25T11:29:26Z

internal/slicer/slicer.go

+func mkWorkdir(workdir string, mode os.FileMode) (err error) {
+	defer func() {
+		if err != nil {
+			err = fmt.Errorf("cannot create working directory: %w", err)


Does this need to be %w?

As this is a "helper" function, I did not want to make too many assumptions on how the returned error was used. In the current implementation, you are right that this is not strictly needed because returned errors are immediately returned as is. However, I wanted to keep consistency with other places in this package where most os errors are wrapped.

This is an internal package, so we are the only ones that could depend in the behavior of this API. If/when we start unwrapping errors returned by Run I would prefer having consistency and avoid being surprised that some errors cannot be unwrapped like others.

Do you agree?

Okay this makes sense, so that the caller could check os.IsExists and the like

This is an internal package, so we are the only ones that could depend in the behavior of this API

In general, the design should not depend too much on whether it is internal or external as the same principles apply. In this case this is dealing with os so it is okay to use %w, in other functions, even if internal, it is not if we don't want to rely on the errors.

letFunny · 2026-03-25T11:31:44Z

internal/slicer/slicer.go

-		// TODO: Use a predetermined name and reserve it. The release validation
-		// would ensure it cannot be used in chisel-releases.
-		tmpWorkDir, err := os.MkdirTemp(targetDir, "chisel-workdir-*")
+		rootfsPath := filepath.Join(rootWorkdirPath, "rootfs")


The naming is a bit weird now, tmpWorkDir indicated this was temporary and a working directory suggesting we will do something to it later to install the files definitely but now rootfsPath loses both connotations.

See my other comment regarding the "working dir" connotation.

However I was on the fence about the "temporary" aspect of it. I am adding at least this aspect back to the name.

internal/slicer/slicer_test.go

letFunny · 2026-03-25T11:38:11Z

internal/slicer/slicer.go

+		// The root working directory must only be removed if empty.
+		// This call will do so or silently fail.
+		os.Remove(rootWorkdirPath)
+	}()


You can also check for error here and return it from the outer function. This might be worth it if we want to differentiate empty dir vs no permissions, the latter is an error.

I did that at first, but then I saw that in the whole code base we never deal with returning the error of the outer function and the error of the deferred function. In the vast majority of cases we even ignore the error of whatever is called in the defer. Returning both errors should be done carefully to not hide the error of the outer function and to make sure the user can make sense of the "complete" error displayed.

I do not like that this is creating a blind spot though, so I would like to discuss it with G.

Just to be clear I meant: if the error is nil but os.Remove fails then we can return the latter. But let's discuss it with G.

letFunny

Thanks Paul, this is almost ready, just a couple of questions.

letFunny · 2026-04-02T07:44:56Z

internal/slicer/slicer_test.go

+	},
+	error: `cannot create working directory: existing entry at .*/\.chisel is not a directory`,
+}, {
+	summary: "Run keeps non-empty .chisel directory",


This might also make it somewhat more flexible for future compatibility. It is a little bit hard to know at this point.

letFunny · 2026-04-02T07:45:38Z

internal/slicer/slicer_test.go

+		err := os.Chmod(opts.TargetDir, 0o555)
+		c.Assert(err, IsNil)
+	},
+	error: `cannot create working directory: mkdir .*/\.chisel: permission denied`,


This is a good error message, thanks.

letFunny · 2026-04-02T07:49:33Z

internal/slicer/slicer_test.go

+	},
+	filesystem: map[string]string{
+		"/.chisel/":     "dir 0755",
+		"/.chisel/keep": "file 0644 6ca7ea2f",


We need to be careful if we ever implement restarts from crashes to use the existence of .chisel to tell us that we can go ahead and resume from a restart. That will break when future versions do not remove this directory.

I am so-so on keeping it or removing it. I see in one of the comments that you state a requirements to not remove it when not empty, can you explain why?

I am so-so on keeping it or removing it. I see in one of the comments that you state a requirements to not remove it when not empty, can you explain why?

That was the agreement with Gustavo. This directory might be used in the future by chisel to store other things that we may want to keep between executions, so already have this logic of keeping it if not empty makes sense.

So given this is a general purpose directory, we should not use its existence as an indicator it is fine to resume operation after a crash. If/when we implement crash recovery, we should probably rely on something inside this directory.

Great! That will work

upils added 2 commits March 25, 2026 11:31

feat: use a predictable working dir

523a540

fix: lint

8ee9abe

upils commented Mar 25, 2026

View reviewed changes

internal/slicer/slicer_test.go Outdated Show resolved Hide resolved

upils marked this pull request as ready for review March 25, 2026 10:44

letFunny reviewed Mar 25, 2026

View reviewed changes

upils added 3 commits March 25, 2026 14:23

fix: apply PR suggestion on rootfs tmp dir name

4889cf7

fix: fail if .chisel path is not a dir

d904b3a

docs: re-add a comment explaining the limit of the solution

afe8dab

letFunny approved these changes Apr 2, 2026

View reviewed changes

Conversation

upils commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

upils commented Mar 25, 2026 •

edited

Loading