feat: creating files logs information for use in chisel.db #105

letFunny · 2023-12-05T09:24:43Z

Have you signed the CLA?

When using the fsutil.Creator, it will store information about the files and directories created. This will then be integrated into a report which will be part of chisel.db in a separate PR.

When creating files as part of the cut command, we will use a proxy that logs information about the hash, path, mode, etc. This will then be ingrated into a report which will be part of chisel.db.

rebornplusplus

The PR looks nice. I left a few comments and suggestions. Let me know what you think.

internal/fsutil/create_test.go

internal/fsutil/proxy_test.go

internal/fsutil/create.go

internal/slicer/report.go

internal/slicer/slicer.go

internal/deb/extract.go

rebornplusplus

Looks trimmed and much better now, thanks!

rebornplusplus · 2024-01-16T10:53:22Z

internal/slicer/slicer.go

@@ -153,11 +153,13 @@ func Run(options *RunOptions) error {
 		if reader == nil {
 			continue
 		}
+		fsCreator := fsutil.NewCreator()


Do we need a different fsCreator for every slice?

Yes. That is one of the bits that can no longer be seen in this trimmed down version of the PR. We need to create one for each because we want to report the files created for each one. If we used the same fsCreator we would not know which slice created which file (unless we resorted to emptying the map or some other hack).

niemeyer

This is looking good, thanks Alberto. Just a few details/questions.

niemeyer · 2024-01-29T10:29:38Z

internal/deb/extract.go

@@ -25,6 +25,7 @@ type ExtractOptions struct {
 	TargetDir string
 	Extract   map[string][]ExtractInfo
 	Globbed   map[string][]string
+	FSCreator *fsutil.Creator


This might be just Creator, given the context, and although this is minor we can use a locally defined interface to allow for variation on the types, even more for testing purposes if required.

Agree on the interface but can we do it when the need arises? Right now the tests depend on the default creator and the tree dump function that inspects the filesystem.

internal/deb/extract.go

internal/fsutil/create.go

internal/fsutil/create_test.go

niemeyer

Just one trivial, and a question about the organization, but otherwise this seems ready for merging.

niemeyer · 2024-02-05T11:00:04Z

internal/deb/extract.go

@@ -58,8 +59,12 @@ func Extract(pkgReader io.Reader, options *ExtractOptions) (err error) {
 	if err != nil {
 		return err
 	}
+	validOpts := *options
+	if validOpts.Creator == nil {
+		validOpts.Creator = fsutil.NewCreator()


This does not seem great as it's doing a shallow copy and we have no idea of the consequences. If there's intended mutation going on to the values of the options, they won't be moved out anymore. If on the other hand, the goal is to not mutate, this won't help in cases where the fields are pointers.

There are multiple options here, but the easiest here seems to be simply forcing the call site to provide the creator. We don't have many call sites anyway, and tests are already doing this in a custom way I believe.

I agree with you. I added it because of your comment about the panic but I agree that, because it is an internal function, we can just add it in each call site.

Yeah, it's a bit of a bind because it'd be fine to either 1) Use a default global value that doesn't increase in memory size over time, or 2) Use a local value that is cheap to create. I'm fine with either, but would prefer to not be creating maps, populating maps, and then destroying (GCing) them on every iteration of a loop. Seems wasteful for no greater good.

niemeyer · 2024-02-05T11:02:31Z

internal/fsutil/create.go

@@ -84,3 +123,20 @@ func createSymlink(o *CreateOptions) error {
 	}
 	return os.Symlink(o.Link, o.Path)
 }
+
+// readerProxy implements the io.Reader interface proxying the calls to its inner io.Reader. On each read, the proxy
+// calculates the file size and hash.


s/calculates/keeps track of/

niemeyer · 2024-02-05T11:06:12Z

internal/setup/fetch.go

@@ -137,7 +137,7 @@ func extractTar(dataReader io.Reader, targetDir string) error {

 		//debugf("Extracting header: %#v", tarHeader)

-		err = fsutil.Create(&fsutil.CreateOptions{
+		err = fsutil.NewCreator().Create(&fsutil.CreateOptions{


This is in a loop, creating and discarding the creator and its map on every iteration, not even keeping track of it. Is the idea here that the follow up PR will unify all of these calls and track these properly?

The idea is that you have one creator per slice to know what files were created for each one. The other option will be cleaning the creator.Created map each iteration to know which files come from the new slice, I think that is more convoluted to follow. From the report PR:

<loop over slice> creator := fsutil.NewCreator() err := deb.Extract(reader, &deb.ExtractOptions{ Package: slice.Package, Extract: extract[slice.Package], TargetDir: targetDir, Globbed: globbedPaths, Creator: creator, }) for _, entry := range creator.Created { report.AddEntry(slice, entry) } <end of loop>

That seems fine. What's a bit awkward is the case above where we have a tight loop around individual file extraction using a new creator every time.

I will change that to the outer possible scope, you are right.

niemeyer · 2024-02-05T11:06:34Z

internal/slicer/slicer.go

@@ -153,11 +153,13 @@ func Run(options *RunOptions) error {
 		if reader == nil {
 			continue
 		}
+		creator := fsutil.NewCreator()


niemeyer · 2024-02-05T11:06:56Z

internal/slicer/slicer.go

@@ -211,7 +213,8 @@ func Run(options *RunOptions) error {
 				return fmt.Errorf("internal error: cannot extract path of kind %q", pathInfo.Kind)
 			}

-			err := fsutil.Create(&fsutil.CreateOptions{
+			creator := fsutil.NewCreator()
+			err := creator.Create(&fsutil.CreateOptions{


... and here.

niemeyer

Please see the replies above as it may be relevant for upcoming PRs, but this seems fine as it is for now. Thanks!

letFunny added 6 commits December 1, 2023 09:46

Add fileCreator proxy for use in chisel.db

a09f06f

When creating files as part of the cut command, we will use a proxy that logs information about the hash, path, mode, etc. This will then be ingrated into a report which will be part of chisel.db.

Tests

ec3690f

Fix tests when running whole suite

a171780

Link

2a1731b

Tests for report

f6346be

Final touches

0295aed

letFunny changed the title ~~Chisel db proxy~~ Add proxy for use in chisel.db Dec 5, 2023

letFunny marked this pull request as ready for review December 5, 2023 09:44

cjdcordeiro requested a review from rebornplusplus December 5, 2023 17:14

Better test coverage

c371d1a

letFunny changed the title ~~Add proxy for use in chisel.db~~ feat: add proxy for use in chisel.db Dec 8, 2023

rebornplusplus reviewed Dec 12, 2023

View reviewed changes

letFunny added 12 commits December 12, 2023 12:44

Address review comments

dfc7647

sort imports

d39f337

remove proxy

dc19319

Merge branch 'main' into chisel-db-proxy

bdf1f70

fix tests

1fdad8a

increase test coverage

5a06152

remove unnecessary tests

b0e0712

sort imports

d47ec8d

remove report to split PRs

c1eb773

simplify naming terminology

1441994

more naming simplification

c8c2ba7

more naming simplification

3b5bb4a

letFunny requested a review from rebornplusplus January 16, 2024 10:16

more naming simplification

13d4b06

letFunny changed the title ~~feat: add proxy for use in chisel.db~~ feat: creating a file logs the hash for use in chisel.db Jan 16, 2024

letFunny mentioned this pull request Jan 16, 2024

feat: add file and directory report skeleton #116

Merged

1 task

rebornplusplus approved these changes Jan 16, 2024

View reviewed changes

letFunny changed the title ~~feat: creating a file logs the hash for use in chisel.db~~ feat: creating files logs information for use in chisel.db Jan 16, 2024

letFunny requested a review from niemeyer January 16, 2024 18:00

cjdcordeiro added the Priority Look at me first label Jan 17, 2024

cjdcordeiro assigned niemeyer Jan 17, 2024

niemeyer requested changes Jan 29, 2024

View reviewed changes

letFunny added 5 commits January 29, 2024 12:16

address review comments

0fccd1f

rename fsCreator

6e62d64

default for missing Creator in options

0737e43

comments

2aa844d

conform to godoc format

5eb8f7e

letFunny requested a review from niemeyer January 29, 2024 11:43

niemeyer requested changes Feb 5, 2024

View reviewed changes

letFunny added 2 commits February 5, 2024 12:20

reword comment

7660433

remove shallow copy of Options

9ee449f

letFunny requested a review from niemeyer February 5, 2024 11:26

niemeyer approved these changes Feb 5, 2024

View reviewed changes

niemeyer merged commit 11a324c into canonical:main Feb 5, 2024
14 checks passed

letFunny deleted the chisel-db-proxy branch October 17, 2024 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: creating files logs information for use in chisel.db #105

feat: creating files logs information for use in chisel.db #105

letFunny commented Dec 5, 2023 •

edited

Loading

rebornplusplus left a comment

rebornplusplus left a comment

rebornplusplus Jan 16, 2024

letFunny Jan 16, 2024

niemeyer left a comment

niemeyer Jan 29, 2024

letFunny Jan 29, 2024

niemeyer left a comment

niemeyer Feb 5, 2024

letFunny Feb 5, 2024

niemeyer Feb 5, 2024

niemeyer Feb 5, 2024

niemeyer Feb 5, 2024

letFunny Feb 5, 2024 •

edited

Loading

niemeyer Feb 5, 2024

letFunny Feb 5, 2024

niemeyer Feb 5, 2024

niemeyer Feb 5, 2024

niemeyer left a comment

feat: creating files logs information for use in chisel.db #105

feat: creating files logs information for use in chisel.db #105

Conversation

letFunny commented Dec 5, 2023 • edited Loading

rebornplusplus left a comment

Choose a reason for hiding this comment

rebornplusplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letFunny Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

letFunny commented Dec 5, 2023 •

edited

Loading

letFunny Feb 5, 2024 •

edited

Loading