Skip to content

Prune stale Protobuf outputs and tighten the protoc up-to-date check - #4923

Open
pepone wants to merge 12 commits into
mainfrom
proto-incremental-build
Open

Prune stale Protobuf outputs and tighten the protoc up-to-date check#4923
pepone wants to merge 12 commits into
mainfrom
proto-incremental-build

Conversation

@pepone

@pepone pepone commented Sep 5, 2026

Copy link
Copy Markdown
Member

Fixes #4817

Stacked on #4922 (base branch proto-clean-item-name), since it builds on the item rename made there. Merge #4922 first; GitHub retargets this PR to main afterwards.

M-17: stale outputs of removed or renamed Proto files

ProtoClean only ever deleted the outputs of the current ProtoFile items, so a removed or renamed .proto left Hello.cs and Hello.IceRpc.cs behind for the SDK's default Compile glob to keep compiling. The targets now mirror the Slice tooling (#4589):

  • A new _ComputeProtoOutputs target computes the absolute paths protoc produces for the current items. ProtoCompile, ProtoClean and the two targets below depend on it. It runs only the name computation, not the up-to-date check, for the reason noted in Use a distinct item name in ProtoClean so Rebuild runs protoc once #4922: on Rebuild it runs before Clean.
  • _ProtoWriteOutputManifest (after ProtoCompile) records those paths in $(IntermediateOutputPath)protoc.outputs.txt.
  • _ProtoPruneStaleOutputs (before ProtoCompile and ProtoClean) deletes anything in the previous manifest that the current build no longer produces, and removes it from Compile first so csc doesn't fail with CS2001. It prunes nothing when _ComputeProtoOutputs was skipped because the task assembly is missing.
  • ProtoClean deletes the manifest and the inputs cache along with the generated files.

M-18: up-to-date check accepting stale outputs

UpToDateCheckTask now:

  • compares the newest input against the oldest output rather than the newest, so an old Hello.IceRpc.cs is no longer masked by a fresh Hello.cs;
  • treats a missing dependency as out of date, instead of reading the placeholder timestamp File.GetLastWriteTime returns for missing files. A deleted import now surfaces as a protoc error rather than a silently stale build;
  • takes AdditionalInputs: the protoc binary, IceRpc.Protobuf.Generator.dll and $(IntermediateOutputPath)protoc.inputs.cache, so a rebuilt or upgraded generator regenerates the code;
  • sees a fresh protoc.inputs.cache when the configuration changes: ProtoCompile writes the compiler and generator versions, the ProtoSearchPath entries and the distinct AdditionalOptions values to it with WriteOnlyWhenDifferent, before the check. The file's timestamp moves only when a value changes; every Proto file is then out of date, including those a failed run left untouched. Adding a Proto file doesn't regenerate the others.

The README note saying that changing AdditionalOptions requires a dotnet clean is replaced by a short description of the incremental behavior.

Verification

One-file probe importing the source-build props/targets (macOS, .NET SDK 10.0.201), protoc runs per build:

Scenario Before After
No change 0 0
Hello.IceRpc.cs older than the .proto, Hello.cs newer 0 (stale) 1
Imported other.proto deleted 0 (stale) protoc error for the missing import
AdditionalOptions changed / unchanged / reverted 0 (stale) 1 / 0 / 1
third.proto added 1 1
hello.proto renamed to greeter.proto 1, Hello.* left behind 1, Hello.* deleted, no duplicate types
Last .proto removed 0, outputs left behind 0, generated/ empty
Generator assembly touched 0 (stale) 1
Fresh Rebuild / second Rebuild / Clean 1 / 1 / outputs deleted 1 / 1 / outputs, manifest and inputs cache deleted

From the branch, IceRpc.Protobuf.BuildTelemetry builds clean, the IceRpc.Protobuf.Tests suite passes (70 tests), and a second build of that test project runs protoc zero times. Not exercised: Windows, where the only platform-specific addition is the .exe extension on the protoc input path.

What's Changed entry

Area: Protobuf

  • Removing or renaming a .proto file, or changing its OutputDir, now deletes the C# files previously generated
    for it during the next build, instead of leaving them for the compiler to pick up.
  • protoc now regenerates a Proto file's code when any of its generated files is older than the Proto file or one of
    its imports, when an import is missing, when protoc or the generator is updated, and when ProtoSearchPath or
    AdditionalOptions changes. Running dotnet clean after changing AdditionalOptions is no longer needed.

ProtoClean and ProtoCompile both wrote their task output to _ProtoFile.
A task <Output> appends to an existing item, and Rebuild runs Clean and
Build in the same project instance, so after a Rebuild _ProtoFile held
two entries per ProtoFile that differed only in the UpToDate metadata.
ProtoCompile then batched protoc over both entries and emitted duplicate
Compile items, which csc reported as CS2002.

ProtoClean now writes to _ProtoCleanFile.

Claude-Session: https://claude.ai/code/session_015iaHfudquGtsULZqXdPKJD
Removing or renaming a .proto file left its generated code behind, and
the SDK's default Compile glob kept compiling it. The build now records
the files protoc produces in $(IntermediateOutputPath)protoc.outputs.txt
and deletes, before CoreCompile and Clean, anything recorded there that
the current ProtoFile items no longer produce, mirroring the Slice
targets.

UpToDateCheckTask also accepted stale outputs. It now compares the newest
input against the oldest output instead of the newest, treats a missing
import as out of date instead of reading its placeholder timestamp, and
takes protoc and the generator assembly as additional inputs. A
fingerprint of the compiler and generator versions, the import search
path and the per-file AdditionalOptions is recorded after each
successful protoc run; a change regenerates every Proto file.

Claude-Session: https://claude.ai/code/session_015iaHfudquGtsULZqXdPKJD
Copilot AI lite review requested due to automatic review settings September 5, 2026 19:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The fingerprint written to disk is MSBuild-escaped but compared against an unescaped value, which will force all Proto files to rebuild every build.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the IceRpc.Protobuf.Tools MSBuild integration to prevent stale generated .cs files from lingering after .proto removals/renames, and to make incremental builds more reliably detect when protoc outputs are out of date (including tool/generator/config changes).

Changes:

  • Add an output manifest + pruning step to delete generated outputs that are no longer produced by the current @(ProtoFile) set.
  • Tighten UpToDateCheckTask to require outputs/inputs to exist, compare newest input vs oldest output, and include additional tool inputs + a configuration fingerprint.
  • Update documentation and add Windows-specific handling for the protoc.exe additional-input path.
File summaries
File Description
src/IceRpc.Protobuf.Tools/UpToDateCheckTask.cs Adds fingerprint/config + additional-input awareness and improves stale-output detection logic.
src/IceRpc.Protobuf.Tools/README.md Updates incremental build documentation to reflect new pruning + up-to-date behavior.
src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets Implements output computation, manifest write, stale-output pruning, and passes new inputs/fingerprint into the up-to-date task.
src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.props Adds Windows .exe extension property used by the new additional-input tracking.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets Outdated
…o-incremental-build

# Conflicts:
#	src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets
@pepone pepone added this to the 0.6.1 milestone Sep 10, 2026
Removing or renaming a .proto file left its generated code behind, and
the SDK's default Compile glob kept compiling it. The build now records
the files protoc produces in $(IntermediateOutputPath)protoc.outputs.txt
and deletes, before CoreCompile and Clean, anything recorded there that
the current ProtoFile items no longer produce, mirroring the Slice
targets.

UpToDateCheckTask also accepted stale outputs. It now compares the newest
input against the oldest output instead of the newest, treats a missing
import as out of date instead of reading its placeholder timestamp, and
takes protoc and the generator assembly as additional inputs. A
fingerprint of the compiler and generator versions, the import search
path and the per-file AdditionalOptions is recorded after each
successful protoc run; a change regenerates every Proto file.

Claude-Session: https://claude.ai/code/session_015iaHfudquGtsULZqXdPKJD
@pepone
pepone changed the base branch from proto-clean-item-name to main September 10, 2026 15:01
string[] outputs =
[
dependOutput,
Path.Combine(OutputDir, $"{fileName}.cs"),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Declining this one. fileName is the item's MSBuild FileName metadata (the file name without extension, converted to PascalCase), so it is never a rooted path and Path.Combine cannot drop OutputDir here. The three calls are unchanged from main; the alert fires because the lines moved.

Comment thread src/IceRpc.Protobuf.Tools/UpToDateCheckTask.cs Fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Configuration-scoped and retained manifests leave stale-output handling incomplete and can delete unrelated replacement files.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets:116

  • When the last ProtoFile is removed, _ProtoWriteOutputManifest is skipped, so this old manifest remains after pruning. Every later build will continue treating the former output paths as owned stale files and can delete a new hand-authored or differently generated file created at one of those paths. Clear the manifest after pruning when there are no current outputs.
    <Delete Files="@(_StaleProtoOutput)" />
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

<Target Name="_ProtoPruneStaleOutputs"
BeforeTargets="ProtoCompile;CoreCompile;ProtoClean;Clean"
DependsOnTargets="_ComputeProtoOutputs"
Condition="Exists('$(IntermediateOutputPath)protoc.outputs.txt')">

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Reproduced: a Debug build, a removed or renamed Proto file, then a Release build compiles the stale output until Debug is built again. The per-configuration/TargetFramework manifest deliberately mirrors IceRpc.Slice.Tools (#4589), so parallel multi-target inner builds do not share one state file. Building several configurations against the same shared OutputDir is a niche setup this PR does not handle. The suppressed comment on line 116 (manifest retained after the last Proto file is removed) is fixed as of 84d0d0a: _ProtoPruneStaleOutputs now deletes the manifest when there are no ProtoFile items.

With two Proto files, changing AdditionalOptions and then hitting a
protoc error on the second file left the first file regenerated with the
new options while protoc.fingerprint.txt still held the previous
fingerprint. Reverting the options then matched the recorded fingerprint
and accepted the first file's outputs as current: protoc ran zero times
for it and the code generated with the reverted options stayed.

UpToDateCheckTask now reports whether the fingerprint changed, and
ProtoCompile deletes the recorded one before running protoc, so a run
that fails part-way leaves no fingerprint for the next build to trust.
_ProtoWriteOutputManifest records the new one once protoc succeeds.
_ProtoWriteOutputManifest only runs when the project has ProtoFile
items, so removing the last Proto file left the previous manifest in
place after its outputs were pruned. Every later build then pruned the
same paths again, deleting any file created there since, such as a
hand-written replacement for the generated code.

_ProtoPruneStaleOutputs now deletes the manifest when there are no
ProtoFile items, after pruning the recorded outputs.
The previous commit added the block a second time, with a typo in its
comment; the first copy came in with the fingerprint commit.

@bernardnormier bernardnormier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The manifest and prune half looks right: it mirrors the Slice tooling, and the Rebuild, failed-protoc and last-file-removed scenarios all check out. My comments are mostly on the fingerprint half, which does more than it needs to, plus a few small things.

The fingerprint machinery (three task parameters, an output property, the conditional Delete of the fingerprint file, the escaped second WriteLinesToFile, and the AfterTargets ordering that keeps them consistent) can be replaced by the mechanism the task already has. Write the fingerprint values to $(IntermediateOutputPath)protoc.inputs.cache with WriteOnlyWhenDifferent="true" before the up-to-date check, and add that file to @(_ProtocInput). The file's mtime moves only when a value changes, every output is then older than an input, and everything regenerates. The partial-failure case falls out too: a rewritten cache stays newer than the outputs protoc didn't get to. This is what the SDK does for csc with CoreCompileInputs.cache. It removes the fingerprint code from UpToDateCheckTask, the Delete step and the Escape call.

Follow-up, not for this PR: IceRpc.Slice.Tools.targets has the bug 84d0d0a fixes here. When the last .slice file is removed, _SlicecWriteOutputManifest doesn't run, slicec.outputs.txt survives, and every later build deletes whatever appears at the former output paths. Worth an issue.

</ItemGroup>
<PropertyGroup>
<!-- Anything that changes the generated code without touching a Proto file; a change regenerates them all. -->
<_ProtocFingerprint>protoc=$(ProtocBundledPluginVersion);icerpc-csharp=$(ProtocIceRpcPluginVersion);search-path=@(ProtoSearchPath->'%(FullPath)', ',');options=@(ProtoFile->'%(Identity)=%(AdditionalOptions)', ',')</_ProtocFingerprint>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

options=@(ProtoFile->'%(Identity)=%(AdditionalOptions)', ',') changes whenever a Proto file is added, removed or renamed, so each of those regenerates every Proto file in the project. On main, adding a file ran protoc once. The README's "Incremental builds" section describes the per-file behavior, not this.

Only the option values need to be in the fingerprint, not the file set: for example the distinct %(AdditionalOptions) values, or a per-file record next to the .d compared in the task.

Verified at this head: adding a Proto file to IceRpc.Protobuf.Tests logs "The protoc configuration changed since the previous build; all Proto files are out of date".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787. The cache records the distinct AdditionalOptions values (@(ProtoFile->'options=%(AdditionalOptions)'->DistinctWithCase()); with-case because Distinct() folds case and protoc options don't). Verified on the branch: adding a Proto file runs protoc once, removing it runs nothing and prunes its outputs. One limitation of the distinct set, for the record: a file switching to an option value another file already uses leaves the set unchanged and isn't regenerated. A per-file record next to the .d would close that at the cost of a manifest entry, clean/prune participation and a compare in the task; say if you'd rather have that.

OutputDir="%(ProtoFile.OutputDir)"
Sources="@(ProtoFile)"
AdditionalInputs="@(_ProtocInput)"
Fingerprint="$(_ProtocFingerprint)"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See the review body: this parameter, FingerprintFile, the FingerprintChanged output, the Delete below and the second WriteLinesToFile in _ProtoWriteOutputManifest all go away if the fingerprint is a cache file listed in @(_ProtocInput).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787: Fingerprint, FingerprintFile, FingerprintChanged, the Delete and the second WriteLinesToFile are gone. protoc.inputs.cache is written in ProtoCompile before the check and listed in @(_ProtocInput).

Comment on lines +101 to +114
<_StaleProtoOutput Include="@(_PreviousProtoOutput)" Exclude="@(_ProtoOutput)" />
<!--
The SDK's default Compile glob records project-relative identities, and <Compile Remove> matches identities
exactly. A MakeRelative call inlined in an item transform doesn't evaluate, hence the separate item.
-->
<_StaleProtoOutputRelative
Include="$([MSBuild]::MakeRelative($(MSBuildProjectDirectory), %(_StaleProtoOutput.Identity)))"
Condition="'%(_StaleProtoOutput.Identity)' != ''" />
<!--
Remove before deleting, or csc fails with CS2001 on the missing files. The absolute form covers explicit
Compile items, the relative form the SDK glob.
-->
<Compile Remove="@(_StaleProtoOutput)" />
<Compile Remove="@(_StaleProtoOutputRelative)" />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MatchOnMetadata matches the SDK glob's relative identities directly, so the relative item, its comment and the MakeRelative call are not needed. The Condition covers the case below.

Suggested change
<_StaleProtoOutput Include="@(_PreviousProtoOutput)" Exclude="@(_ProtoOutput)" />
<!--
The SDK's default Compile glob records project-relative identities, and <Compile Remove> matches identities
exactly. A MakeRelative call inlined in an item transform doesn't evaluate, hence the separate item.
-->
<_StaleProtoOutputRelative
Include="$([MSBuild]::MakeRelative($(MSBuildProjectDirectory), %(_StaleProtoOutput.Identity)))"
Condition="'%(_StaleProtoOutput.Identity)' != ''" />
<!--
Remove before deleting, or csc fails with CS2001 on the missing files. The absolute form covers explicit
Compile items, the relative form the SDK glob.
-->
<Compile Remove="@(_StaleProtoOutput)" />
<Compile Remove="@(_StaleProtoOutputRelative)" />
<_StaleProtoOutput Include="@(_PreviousProtoOutput)" Exclude="@(_ProtoOutput)"
Condition="'@(_ProtoOutput)' != '' Or '@(ProtoFile)' == ''" />
<!-- Remove before deleting, or csc fails with CS2001 on the missing files. -->
<Compile Remove="@(_StaleProtoOutput)" MatchOnMetadata="FullPath" MatchOnMetadataOptions="PathLike" />

Verified with SDK 10.0: this removes both the glob's generated/Stale.cs and an absolute Compile entry. It also fixes a real, if unlikely, failure: the unquoted MakeRelative(..., %(Identity)) splits on a comma in the path and fails with MSB4186. The Slice copy has the same shape.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Applied in e323787. Verified with SDK 10.0.201: renaming hello.proto to greeter.proto deletes Hello.*, removes the glob's Compile entry and the build succeeds.

<Target Name="_ProtoPruneStaleOutputs"
BeforeTargets="ProtoCompile;CoreCompile;ProtoClean;Clean"
DependsOnTargets="_ComputeProtoOutputs"
Condition="Exists('$(IntermediateOutputPath)protoc.outputs.txt')">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When _ComputeProtoOutputs is skipped by its Exists condition while ProtoFile is not empty (source build, --no-dependencies after the Tools bin was cleaned), _ProtoOutput is empty, so every recorded output is treated as stale and deleted, and only then does ProtoCompile fail to load the task. The old ProtoCompile failed the same way but left the generated code alone. The Condition on _StaleProtoOutput in the suggestion below guards it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Applied in e323787. Verified: with the task assembly path pointing at a missing directory and --no-dependencies, _ProtoPruneStaleOutputs runs, deletes nothing, and the build fails on the task load with the generated code intact. ProtoClean's manifest Delete got the same condition: in a source build dotnet clean cleans the Tools project first, and deleting the manifest there would leave a removed Proto file's outputs unpruned by the next build.

Comment on lines +89 to +94
<!--
BeforeTargets includes CoreCompile and Clean for the case where ProtoFile is empty: ProtoCompile and ProtoClean
are then skipped, but the outputs of a previous build still need pruning.
-->
<Target Name="_ProtoPruneStaleOutputs"
BeforeTargets="ProtoCompile;CoreCompile;ProtoClean;Clean"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BeforeTargets run before their target even when that target's Condition is false, and ProtoClean has no Condition at all now, so CoreCompile and Clean in the list are redundant and the comment's reason doesn't hold.

Suggested change
<!--
BeforeTargets includes CoreCompile and Clean for the case where ProtoFile is empty: ProtoCompile and ProtoClean
are then skipped, but the outputs of a previous build still need pruning.
-->
<Target Name="_ProtoPruneStaleOutputs"
BeforeTargets="ProtoCompile;CoreCompile;ProtoClean;Clean"
<!-- Runs even when ProtoFile is empty: the outputs of a previous build still need pruning. -->
<Target Name="_ProtoPruneStaleOutputs"
BeforeTargets="ProtoCompile;ProtoClean"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Applied in e323787. Verified: with the last Proto file removed, the target still prunes the outputs and deletes the manifest.

AfterTargets="ProtoCompile"
DependsOnTargets="_ComputeProtoOutputs"
Condition="@(ProtoFile) != ''">
<MakeDir Directories="$(IntermediateOutputPath)" />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WriteLinesToFile creates the directory, and PrepareForBuild already created it. Drop the line.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787.

Comment on lines +229 to 235
<Target Name="ProtoClean" BeforeTargets="Clean" DependsOnTargets="_ComputeProtoOutputs">
<Delete Files="@(_ProtoOutput)" />
<Delete Files="$(IntermediateOutputPath)protoc.outputs.txt"
Condition="Exists('$(IntermediateOutputPath)protoc.outputs.txt')" />
<Delete Files="$(IntermediateOutputPath)protoc.fingerprint.txt"
Condition="Exists('$(IntermediateOutputPath)protoc.fingerprint.txt')" />
</Target>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delete skips missing files (the first Delete already relies on that), so the Exists guards are redundant and one task does it. Same for the Exists half of the condition on the Delete in ProtoCompile, moot if the fingerprint becomes a cache file.

Suggested change
<Target Name="ProtoClean" BeforeTargets="Clean" DependsOnTargets="_ComputeProtoOutputs">
<Delete Files="@(_ProtoOutput)" />
<Delete Files="$(IntermediateOutputPath)protoc.outputs.txt"
Condition="Exists('$(IntermediateOutputPath)protoc.outputs.txt')" />
<Delete Files="$(IntermediateOutputPath)protoc.fingerprint.txt"
Condition="Exists('$(IntermediateOutputPath)protoc.fingerprint.txt')" />
</Target>
<Target Name="ProtoClean" BeforeTargets="Clean" DependsOnTargets="_ComputeProtoOutputs">
<Delete Files="@(_ProtoOutput)" />
<Delete Files="$(IntermediateOutputPath)protoc.outputs.txt;$(IntermediateOutputPath)protoc.fingerprint.txt" />
</Target>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787; the Delete now covers protoc.inputs.cache instead of the fingerprint.

return false;
}

// Every output must be newer than every input, so compare against the oldest output.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The second clause narrates the Min below it.

Suggested change
// Every output must be newer than every input, so compare against the oldest output.
// Every output must be newer than every input.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787.

Comment thread src/IceRpc.Protobuf.Tools/README.md Outdated
Comment on lines +80 to +83
`protoc` runs only for the Proto files whose generated code is missing or out of date. A Proto file is out of date
when the Proto file itself, one of the files it imports, `protoc` or the `protoc-gen-icerpc-csharp` generator is newer
than one of its generated files. Changing `AdditionalOptions` or `ProtoSearchPath`, or upgrading this package,
regenerates the code of all Proto files.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

protoc doesn't run on its own; the build runs it.

Suggested change
`protoc` runs only for the Proto files whose generated code is missing or out of date. A Proto file is out of date
when the Proto file itself, one of the files it imports, `protoc` or the `protoc-gen-icerpc-csharp` generator is newer
than one of its generated files. Changing `AdditionalOptions` or `ProtoSearchPath`, or upgrading this package,
regenerates the code of all Proto files.
The build runs `protoc` only for the Proto files whose generated code is missing or out of date. A Proto file is
out of date when the Proto file itself, one of the files it imports, `protoc` or the `protoc-gen-icerpc-csharp`
generator is newer than one of its generated files. Changing `AdditionalOptions` or `ProtoSearchPath`, or upgrading
this package, regenerates the code of all Proto files.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787.

Comment thread src/IceRpc.Protobuf.Tools/README.md Outdated
Comment on lines +85 to +86
The generated code of a Proto file that is removed from the project, renamed, or given a different `OutputDir` is
deleted during the next build.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Three stacked qualifiers on a passive sentence; active voice reads better.

Suggested change
The generated code of a Proto file that is removed from the project, renamed, or given a different `OutputDir` is
deleted during the next build.
When you remove a Proto file from the project, rename it, or change its `OutputDir`, the next build deletes the code
previously generated for it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 Claude: Done in e323787.

Review feedback on #4923:

- ProtoCompile writes the protoc and generator versions, the ProtoSearchPath
  entries and the distinct AdditionalOptions values to
  $(IntermediateOutputPath)protoc.inputs.cache with WriteOnlyWhenDifferent and
  lists the file in @(_ProtocInput). UpToDateCheckTask loses Fingerprint,
  FingerprintFile and FingerprintChanged, ProtoCompile the conditional Delete,
  and _ProtoWriteOutputManifest the escaped second WriteLinesToFile.
- Only the distinct option values are part of the cache, so adding, removing
  or renaming a Proto file no longer regenerates the others.
- _ProtoPruneStaleOutputs removes stale Compile items with
  MatchOnMetadata="FullPath" instead of a MakeRelative item, skips pruning
  when _ComputeProtoOutputs was skipped, and hooks only ProtoCompile and
  ProtoClean.
- ProtoClean keeps the manifest when _ComputeProtoOutputs was skipped and
  drops the Exists guards; _ProtoWriteOutputManifest drops the MakeDir.
- README wording and comment fixes.
@pepone

pepone commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

🤖 Claude: Done in e323787: the fingerprint machinery is replaced by protoc.inputs.cache, written with WriteOnlyWhenDifferent before the up-to-date check and listed in @(_ProtocInput). Verified with a two-file probe against the branch (protoc runs per build): unchanged 0; options changed / unchanged / reverted 2 / 0 / 2; search path changed 2; Proto file added 1; a cache change with protoc failing on one file, then that file fixed, regenerates only that file; Rebuild twice 2 / 2; Clean deletes outputs, manifest and cache. IceRpc.Protobuf.Tests passes and a second build runs protoc 0 times. The Slice follow-up is #4930.

[
dependOutput,
Path.Combine(OutputDir, $"{fileName}.cs"),
Path.Combine(OutputDir, $"{fileName}.IceRpc.cs"),
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Audit-Medium] Protobuf incremental build accepts stale outputs and never prunes removed inputs

3 participants