refactor(protogen): sweep stale protos by ownership marker instead of deleting upfront - #451
Open
Kybxd wants to merge 3 commits into
Open
refactor(protogen): sweep stale protos by ownership marker instead of deleting upfront#451Kybxd wants to merge 3 commits into
Kybxd wants to merge 3 commits into
Conversation
… deleting upfront GenAll used to empty the outdir before generating anything, keeping only the files listed in the ProtoFiles option, which had several problems: - The protection list was derived from an unrelated option: ProtoFiles specifies the predefined proto files to compile, yet it doubled as the deletion allowlist, so a handwritten proto file living directly in the outdir was removed unless it happened to be listed there. Notably it was always removed in descriptor set mode, where ProtoFiles is unset. - Deleting before validating was destructive: a run failing halfway left the outdir emptied. Together with preserveFieldNumbers, whose warm-up cache is in-memory only, the next run had no field number baseline any more, hence silently reassigned field numbers from 1 and broke binary compatibility, which is exactly what the option is meant to prevent. - Conflict detection relied on emptying the outdir beforehand, as two books generating the same proto file were detected by testing the existence of the file. So it was unavailable to GenWorkbook, where such a conflict silently overwrote the previously generated one. Now the proto files generated in this run are tracked in memory, which is the source of truth for both concerns: - Conflict detection queries the tracked paths instead of the filesystem, so it always applies, and both conflicting books are named in the error message. - After all proto files are generated successfully, the outdir is swept recursively to remove the ones generated by previous runs but not by this one, identified by the generated file header that protogen always writes. Hence the handwritten proto files are kept no matter where they live, and a failed run leaves the previously generated ones intact. The sweep only happens in GenAll, which is authoritative over the whole outdir, whereas GenWorkbook generates only the specified books, so the others are not stale. Thus both the delExisted and checkProtoFileConflicts parameters are gone. The conflict is reported as the new error code E1000, so as to be localized. It names the workbook paths rather than the workbook names, as the latter are the snake-cased basenames, which are always the same for the conflicting workbooks, hence useless for telling them apart. NOTE: the preserveFieldNumbers snapshot in preprocess is kept as defense in depth. The generated proto files are truncated in place during generation, and an exporter happens to query the snapshot, which initializes it lazily, before truncating its own file. But a workbook whose worksheets are all filtered out truncates its file without querying at all, which would otherwise let another exporter snapshot a truncated file, hence silently reassign field numbers.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #451 +/- ##
==========================================
- Coverage 75.64% 75.63% -0.01%
==========================================
Files 88 88
Lines 9533 9599 +66
==========================================
+ Hits 7211 7260 +49
- Misses 1747 1759 +12
- Partials 575 580 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
GenAllused to empty the outdir before generating anything, keeping only the files listed inProtoFiles. Three problems came out of that.1. The protection list was derived from an unrelated option
ProtoFilesspecifies the predefined proto files to compile, yet it doubled as the deletion allowlist:So a handwritten proto file living directly in the outdir was removed unless it happened to be listed there.
2. Deleting before validating was destructive
A run failing halfway left the outdir emptied. Combined with
preserveFieldNumbers, whose snapshot is in-memory only, the next run had no field number baseline any more, hence silently reassigned field numbers from 1 and broke binary compatibility — exactly what the option exists to prevent.3. Conflict detection depended on the outdir being emptied
Two workbooks generating the same proto file were detected by testing the existence of the file:
This only works if the outdir was emptied first, so it was unavailable to
GenWorkbook, where such a conflict silently overwrote the previously generated file. NotefilenameWithSubdirPrefixdefaults tofalse, soexcel/a/Item.xlsxandexcel/b/Item.xlsxdo collide by default.Solution
The proto files generated in this run are now tracked in memory, which becomes the source of truth for both concerns.
Conflict detection queries the tracked paths instead of the filesystem, so it always applies, and both conflicting workbooks are named. Reported as the new error code
E1000so it can be localized:It deliberately names the workbook paths, not
wb.GetName()— the latter is the snake-cased basename, which is always identical for conflicting workbooks and therefore useless for telling them apart.Sweeping happens after all proto files are generated successfully, removing recursively the ones generated by previous runs but not by this one, identified by the header protogen always writes:
The version part is intentionally excluded, so protos generated by other versions of tableau are still recognized. The exporter now emits the header from this same constant, so the two can never drift.
The sweep only happens in
GenAll, which is authoritative over the whole outdir;GenWorkbookgenerates only the specified workbooks, so the others are not stale.Thus both the
delExistedandcheckProtoFileConflictsparameters are gone, andprepareOutdirdegenerates intoMkdirAll.Outcome
ProtoFilesGenAllonlyGenAll+GenWorkbookfile already exists: xxxE1000, names both workbook pathsThis also retroactively fixes a latent issue for descriptor set mode (#TBD), where
ProtoFilesis unset and every proto in the outdir would have been deleted.Behavior changes
.protowithout the tableau generated header is no longer removed byGenAll. More correct, but a change.Tests
Test_sweepOutdir_keepsHandwrittenProtoInOutdir— guards problem 1 directlyTest_sweepOutdir_removesStaleGeneratedProtoTest_registerGeneratedProtoFile_conflict— assertsE1000and that both distinct paths appearTest_isGeneratedProtoFile— 6 subtests incl. another tableau version,protoc-gen-go, empty and truncated filesTest_sweepOutdir/Test_prepareOutdir— rewritten for the new semanticsFull
go test ./...andgo test -race ./internal/protogen/pass.Verified end-to-end with
cmd/tableauc: a stale generated proto is swept, a handwritten one is kept, and the conflict scenario reproduces theE1000message above in both locales.Note
"A failed run leaves the previous protos intact" is guaranteed structurally (
processSecondPassreturns early on error,sweepOutdirruns after it) and covered by unit tests, but I could not construct an end-to-end failing run to demonstrate it.