Repository navigation
feat: internal manifest package - #144
Conversation
letFunny
commented
Jul 2, 2024
letFunny
commented
- Have you signed the CLA?
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
This is looking very good, Alberto.
Many comments, but nothing fundamental.
rebornplusplus
left a comment
rebornplusplus
left a comment
There was a problem hiding this comment.
Looks good to me overall, only a few questions and suggestions.
|
Discussed offline with @niemeyer some parts of the PR. We agreed to:
EDIT: I have done the tests for the second point and using pointers and a closure means that Go is allocating memory because of the escape analysis, i.e. it cannot figure out if the pointer will escape from the closure. To be discussed further, but keeping the structs for now. |
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks for the tuning, Alberto. Final review, I believe.
| return iteratePrefix(manifest, Path{Kind: "path", Path: pathPrefix}, onMatch) | ||
| } | ||
|
|
||
| func (manifest *Manifest) IteratePackages(onMatch func(Package) error) (err error) { |
There was a problem hiding this comment.
We talked about using pointers for these but it's still using values. Is there a reason to keep these, or maybe I just misunderstood the plan?
There was a problem hiding this comment.
As discussed on the meeting last week, I did the full escape analysis on the real code and not synthetically with some closures. If I am interpreting it correctly, it turns out that passing by value or by pointer in this case does not matter because inside iteratePrefix we are passing it to functions that expect any, which means the value is moved to the heap and it does not matter whether it is a pointer or a struct. I have written a somewhat more lengthy discussion with the commands I have used and the output of the compiler here if you want to double check.
I have changed the code to use pointers because we are moving the values to the heap anyway and we avoid one of the copies.
|
|
||
| // Read loads a Manifest from a file without performing any validation. The file | ||
| // is assumed to be both valid jsonwall and a valid Manifest (see [Validate]). | ||
| func Read(absPath string) (manifest *Manifest, err error) { |
There was a problem hiding this comment.
I've missed this on the first review: why is this not an io.Reader? It seems curious that it's not only a filename, but a filename to a zstd compressed file. Doesn't feel like this package should care about the compression format of the manifest, or how the manifest is being read from disk. Note how the jsonwall.ReadDB function blended nicely below. We won't be able to do this with this function.
There was a problem hiding this comment.
Agree I was thinking that zstd was part of the "format" but there is nothing dictating that, we could be storing it in a completely different way and this package should continue working correctly. The initial idea was to get the filename to be able to do some more things in the validation but I think that is better suited for slicer not for the general manifest.
I have made the changes in https://github.com/canonical/chisel/pull/144/files/56dd49ca33944ab4e09cc7778cdaec7245732924..4e0e33b13cd8319e31fb0a0c36587cfa479dfd7c.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks for the tuning, Alberto. This looks great.
