Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cmd/chisel/cmd_cut.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/canonical/chisel/internal/cache"
"github.com/canonical/chisel/internal/setup"
"github.com/canonical/chisel/internal/slicer"
"github.com/canonical/chisel/public/manifest"
)

var shortCutHelp = "Cut a tree with selected slices"
Expand Down Expand Up @@ -73,6 +74,24 @@ func (cmd *cmdCut) Execute(args []string) error {
}
}

mfest, err := slicer.SelectValidManifest(cmd.RootDir, release)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general our philosophy is that we should be able to cut a release from main at any point in time. That is, if we ever do a bug fix we should be able to release a point version. This is changing how Chisel works in folders with a manifest and it is not the correct behavior (yet) so I would prefer if we could gate this functionality over a environmental variable or a debug command or any other mechanism.

if err != nil {
return err
}
if mfest != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like how all this code ended up looking like. It took several iterations but I am quite happy with the result now.

err = mfest.IterateSlices("", func(slice *manifest.Slice) error {
sk, err := setup.ParseSliceKey(slice.Name)
if err != nil {
return err
}
sliceKeys = append(sliceKeys, sk)
return nil
})
if err != nil {
return err
}
}

selection, err := setup.Select(release, sliceKeys, cmd.Arch)
if err != nil {
return err
Expand Down Expand Up @@ -125,6 +144,7 @@ func (cmd *cmdCut) Execute(args []string) error {
Selection: selection,
Archives: archives,
TargetDir: cmd.RootDir,
Manifest: mfest,
})
return err
}
2 changes: 1 addition & 1 deletion internal/deb/extract_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ var extractTests = []extractTest{{
}},
},
},
error: `cannot extract from package "test-package": cannot create path /[a-z0-9\-\/]*/file outside of root /[a-z0-9\-\/]*`,
error: `cannot extract from package "test-package": cannot handle path /[a-z0-9\-\/]*/file outside of root /[a-z0-9\-\/]*`,
}}

func (s *S) TestExtract(c *C) {
Expand Down
27 changes: 19 additions & 8 deletions internal/fsutil/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type Entry struct {
//
// Create can return errors from the os package.
func Create(options *CreateOptions) (*Entry, error) {
o, err := getValidOptions(options)
o, err := getValidCreateOptions(options)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -124,7 +124,7 @@ func Create(options *CreateOptions) (*Entry, error) {
// information recorded in Entry. The Hash and Size attributes are set on
// calling Close() on the Writer.
func CreateWriter(options *CreateOptions) (io.WriteCloser, *Entry, error) {
o, err := getValidOptions(options)
o, err := getValidCreateOptions(options)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -166,11 +166,22 @@ func createDir(o *CreateOptions) error {
if err != nil {
return err
}
err = os.Mkdir(path, o.Mode)
if os.IsExist(err) {
return nil
fileinfo, err := os.Lstat(path)
if err == nil {
if fileinfo.IsDir() {
if fileinfo.Mode() != o.Mode && o.OverrideMode {
return os.Chmod(path, o.Mode)
}
return nil
}
err = os.Remove(path)
if err != nil {
return err
}
} else if !os.IsNotExist(err) {
return err
}
return err
return os.Mkdir(path, o.Mode)
Comment on lines +169 to +184
Copy link
Collaborator

Choose a reason for hiding this comment

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

A couple of things:

  • Prefer early return when possible. In this case it is better to check if os.IsNotExist and fail directly then the rest of the block does not need to be indented.
  • It is now a bit wasteful to do Lstat here followed by another one in fsutil.Create. It would be great if we could unify them.
  • Are we sure we want to remove the path here? This is "breaking compatibility" with the past and it might lead to unexpected deletion in the user side.
  • This feels unrelated to the PR. Maybe as a separate PR (this one is 1400+ lines) and you can state a TODO in this one saying we found a bug in the existing code that will be fixed next.

}

func createFile(o *CreateOptions) error {
Expand Down Expand Up @@ -241,7 +252,7 @@ func createHardLink(o *CreateOptions) error {
return err
}

func getValidOptions(options *CreateOptions) (*CreateOptions, error) {
func getValidCreateOptions(options *CreateOptions) (*CreateOptions, error) {
optsCopy := *options
o := &optsCopy
if o.Root == "" {
Expand All @@ -257,7 +268,7 @@ func getValidOptions(options *CreateOptions) (*CreateOptions, error) {
func absPath(root, relPath string) (string, error) {
path := filepath.Clean(filepath.Join(root, relPath))
if !strings.HasPrefix(path, root) {
return "", fmt.Errorf("cannot create path %s outside of root %s", path, root)
return "", fmt.Errorf("cannot handle path %s outside of root %s", path, root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to change the error message? It feels unrelated to the PR.

}
return path, nil
}
Expand Down
4 changes: 2 additions & 2 deletions internal/fsutil/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ var createTests = []createTest{{
Mode: 0666,
Data: bytes.NewBufferString("hijacking system file"),
},
error: `cannot create path /file outside of root /root/`,
error: `cannot handle path /file outside of root /root/`,
}, {
summary: "Hardlink cannot escape Root",
options: fsutil.CreateOptions{
Expand Down Expand Up @@ -410,7 +410,7 @@ var createWriterTests = []createWriterTest{{
Mode: 0644,
MakeParents: true,
},
error: `cannot create path /file outside of root /root/`,
error: `cannot handle path /file outside of root /root/`,
}}

func (s *S) TestCreateWriter(c *C) {
Expand Down
84 changes: 84 additions & 0 deletions internal/fsutil/move.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package fsutil

import (
"fmt"
"io/fs"
"os"
"path/filepath"
)

type MoveOptions struct {
SrcRoot string
DstRoot string
// Path is relative to Root.
Path string
Mode fs.FileMode
// If MakeParents is true, missing parent directories of Path are
// created with permissions 0755.
MakeParents bool
// If OverrideMode is true and entry already exists, update the mode. Does
// not affect symlinks.
OverrideMode bool
}

// Move moves or create a filesystem entry according to the provided options.
//
// Move can return errors from the os package.
func Move(options *MoveOptions) error {
o, err := getValidMoveOptions(options)
if err != nil {
return err
}

srcPath, err := absPath(options.SrcRoot, o.Path)
if err != nil {
return err
}
dstPath, err := absPath(options.DstRoot, o.Path)
if err != nil {
return err
}

if o.MakeParents {
if err := os.MkdirAll(filepath.Dir(dstPath), 0o755); err != nil {
return err
}
}

switch o.Mode & fs.ModeType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the API it feels a bit premature to make it this general where in reality we are creating folders (and calling rmdir) and renaming files. Let's have the discussion and don't do anything yet, but what do you think about inlining it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So far slicer was not really concerned about how content is actually created on disk, but dealt with higher level concepts such as "creating an fs entry". I wanted to respect this abstraction. As stated in my other comment on remove.go I also implemented all of this in upgrade at first but it was inconsistent because on one hand content was created with fsutil.Create but moved/deleted with functions from os.

case 0, fs.ModeSymlink:
err = os.Rename(srcPath, dstPath)
case fs.ModeDir:
err = createDir(&CreateOptions{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not moving the directory, it is left behind. It is also unexpected when an API is called Move and it moves the directory without its contents.

Root: o.DstRoot,
Path: o.Path,
Mode: o.Mode,
OverrideMode: o.OverrideMode,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should always be true or else we are not moving anything, it shouldn't be an option of Move.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point.

})
default:
err = fmt.Errorf("unsupported file type: %s", o.Path)
}
if err != nil {
return err
}

return nil
}

func getValidMoveOptions(options *MoveOptions) (*MoveOptions, error) {
optsCopy := *options
o := &optsCopy
if o.SrcRoot == "" {
return nil, fmt.Errorf("internal error: MoveOptions.SrcRoot is unset")
}
if o.DstRoot == "" {
return nil, fmt.Errorf("internal error: MoveOptions.DstRoot is unset")
}
if o.SrcRoot != "/" {
o.SrcRoot = filepath.Clean(o.SrcRoot) + "/"
}
if o.DstRoot != "/" {
o.DstRoot = filepath.Clean(o.DstRoot) + "/"
}
return o, nil
}
Loading
Loading