Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
looking good; ideally I'd love to use something like this to clean up Makefile targets like this one where we are doing multiple generations and can't use clean because of the repeat buf generate calls. (probably still can't update that one because of the make variables, but this will still be great.)
Just a couple suggestions on my end.
| if err == nil && bytes.Equal(existingContent, newContent) { | ||
| return nil | ||
| } | ||
| return os.WriteFile(outFilePath, newContent, 0600) |
There was a problem hiding this comment.
just checking: is this matching the behavior of os.Create? just wondering about the 0600
| // isArchivePath returns true if the given path has a .zip or .jar extension. | ||
| func isArchivePath(path string) bool { | ||
| ext := filepath.Ext(path) | ||
| return ext == ".zip" || ext == ".jar" |
There was a problem hiding this comment.
seems like we should consolidate these extensions into some consts/seqs so we don't miss updating them?
There was a problem hiding this comment.
Made consts in this package. It is used outside, but rarely.
| outDirPath string, | ||
| retainPaths map[string]struct{}, | ||
| ) error { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
nit: should thread through the ctx, even if Close doesn't take one?
There was a problem hiding this comment.
Changed interface to Close(ctx context.Context) error
There was a problem hiding this comment.
fwiw, I'd be fine with leaving them as io.Closers and just having a single context.Background() that gets passed down within the Closer itself. But the update you made also makes sense to me. Just figured we ought to share one.
2 participants
This implements smart clean for
buf generate --cleanas described in this comment, building on the mtime-preserving skip-unchanged behavior from #4396.With --clean, buf generate now handles each output directory intelligently:
This replaces the previous delete-everything-then-regenerate approach with a compare-and-reconcile model, which is safer for mtime-based build systems (make, cargo, ninja) and avoids unnecessary downstream rebuilds.
Architecture change: the
ResponseWriteris now created once perGenerate()call rather than per image. This is required so the writer can see the full set of generated paths across all images before deciding what to delete. A side effect is improved atomicity: if any plugin fails, no partial output is written. The trade-off is holding all images' output inmemory until flush, which should be negligible in practice.
Fixes #3126