-
Notifications
You must be signed in to change notification settings - Fork 13
Prune stale Protobuf outputs and tighten the protoc up-to-date check #4923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
0004aeb
491aa3f
c2e116d
f39b269
b35f57d
1e58b0d
7a74132
5e259e6
fa85e97
84d0d0a
e323787
ef31512
971c1e2
cb67c60
033a19c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -60,12 +60,94 @@ | |||||||||||||||||||||||||||||||||||||
| Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" | ||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||
| </ItemGroup> | ||||||||||||||||||||||||||||||||||||||
| <Target Name="ProtoCompile" BeforeTargets="CoreCompile" Condition="@(ProtoFile) != ''"> | ||||||||||||||||||||||||||||||||||||||
| <!-- | ||||||||||||||||||||||||||||||||||||||
| This target must not run the up-to-date check: Rebuild runs Clean then Build in the same project instance, so a | ||||||||||||||||||||||||||||||||||||||
| check made before Clean would mark as current the outputs Clean is about to delete. ProtoCompile keeps its own | ||||||||||||||||||||||||||||||||||||||
| _ProtoFile item because a task <Output> appends to an existing item. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| In a source build the task assembly doesn't exist until IceRpc.Protobuf.Tools is built, hence the Exists check. | ||||||||||||||||||||||||||||||||||||||
| --> | ||||||||||||||||||||||||||||||||||||||
| <Target Name="_ComputeProtoOutputs" | ||||||||||||||||||||||||||||||||||||||
| Condition="@(ProtoFile) != '' And Exists('$(IceRpcProtobufToolsTaskAssembliesPath)IceRpc.Protobuf.Tools.dll')"> | ||||||||||||||||||||||||||||||||||||||
| <OutputFileNamesTask Sources="@(ProtoFile)"> | ||||||||||||||||||||||||||||||||||||||
| <Output ItemName="_ProtoNamedFile" TaskParameter="ComputedSources" /> | ||||||||||||||||||||||||||||||||||||||
| </OutputFileNamesTask> | ||||||||||||||||||||||||||||||||||||||
| <ItemGroup> | ||||||||||||||||||||||||||||||||||||||
| <_ProtoOutput Include="$([MSBuild]::NormalizePath('%(_ProtoNamedFile.OutputDir)/%(_ProtoNamedFile.OutputFileName).cs'))" /> | ||||||||||||||||||||||||||||||||||||||
| <_ProtoOutput Include="$([MSBuild]::NormalizePath('%(_ProtoNamedFile.OutputDir)/%(_ProtoNamedFile.OutputFileName).IceRpc.cs'))" /> | ||||||||||||||||||||||||||||||||||||||
| <_ProtoOutput Include="$([MSBuild]::NormalizePath('%(_ProtoNamedFile.OutputDir)/%(_ProtoNamedFile.OutputFileName).d'))" /> | ||||||||||||||||||||||||||||||||||||||
| <_ProtoOutput | ||||||||||||||||||||||||||||||||||||||
| Include="$([MSBuild]::NormalizePath('%(_ProtoNamedFile.OutputDir)/%(_ProtoNamedFile.OutputFileName).BuildTelemetry.txt'))" | ||||||||||||||||||||||||||||||||||||||
| Condition="'$(IceRpcBuildTelemetry)' == 'true' And '$(IceRpcBuildTelemetryDebug)' == 'true'" /> | ||||||||||||||||||||||||||||||||||||||
| </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> | ||||||||||||||||||||||||||||||||||||||
| </PropertyGroup> | ||||||||||||||||||||||||||||||||||||||
| </Target> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <!-- | ||||||||||||||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||||||||||||||||||||||
| DependsOnTargets="_ComputeProtoOutputs" | ||||||||||||||||||||||||||||||||||||||
| Condition="Exists('$(IntermediateOutputPath)protoc.outputs.txt')"> | ||||||||||||||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||||||||||||
| <ReadLinesFromFile File="$(IntermediateOutputPath)protoc.outputs.txt"> | ||||||||||||||||||||||||||||||||||||||
| <Output TaskParameter="Lines" ItemName="_PreviousProtoOutput" /> | ||||||||||||||||||||||||||||||||||||||
| </ReadLinesFromFile> | ||||||||||||||||||||||||||||||||||||||
| <ItemGroup> | ||||||||||||||||||||||||||||||||||||||
| <_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)" /> | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Verified with SDK 10.0: this removes both the glob's
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Applied in e323787. Verified with SDK 10.0.201: renaming |
||||||||||||||||||||||||||||||||||||||
| </ItemGroup> | ||||||||||||||||||||||||||||||||||||||
| <Delete Files="@(_StaleProtoOutput)" /> | ||||||||||||||||||||||||||||||||||||||
| <!-- | ||||||||||||||||||||||||||||||||||||||
| With no ProtoFile left, _ProtoWriteOutputManifest doesn't run to replace the manifest; delete it here, or | ||||||||||||||||||||||||||||||||||||||
| every later build deletes whatever appears at the former output paths. | ||||||||||||||||||||||||||||||||||||||
| --> | ||||||||||||||||||||||||||||||||||||||
|
pepone marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||
| <Delete Files="$(IntermediateOutputPath)protoc.outputs.txt" Condition="@(ProtoFile) == ''" /> | ||||||||||||||||||||||||||||||||||||||
| </Target> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <Target Name="ProtoCompile" BeforeTargets="CoreCompile" DependsOnTargets="_ComputeProtoOutputs" Condition="@(ProtoFile) != ''"> | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 129 columns.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787. |
||||||||||||||||||||||||||||||||||||||
| <ItemGroup> | ||||||||||||||||||||||||||||||||||||||
| <!-- Tools whose update must regenerate the code, in addition to the inputs recorded in the dependency files. --> | ||||||||||||||||||||||||||||||||||||||
| <_ProtocInput Include="$(IceRpcProtocPath)$(IceRpcProtocPrefix)/protoc$(_ProtocExecutableExtension)" /> | ||||||||||||||||||||||||||||||||||||||
| <_ProtocInput Include="$(IceRpcProtocGenPath)IceRpc.Protobuf.Generator.dll" /> | ||||||||||||||||||||||||||||||||||||||
| </ItemGroup> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <MakeDir Directories="%(ProtoFile.OutputDir)" /> | ||||||||||||||||||||||||||||||||||||||
| <UpToDateCheckTask OutputDir="%(ProtoFile.OutputDir)" Sources="@(ProtoFile)"> | ||||||||||||||||||||||||||||||||||||||
| <UpToDateCheckTask | ||||||||||||||||||||||||||||||||||||||
| OutputDir="%(ProtoFile.OutputDir)" | ||||||||||||||||||||||||||||||||||||||
| Sources="@(ProtoFile)" | ||||||||||||||||||||||||||||||||||||||
| AdditionalInputs="@(_ProtocInput)" | ||||||||||||||||||||||||||||||||||||||
| Fingerprint="$(_ProtocFingerprint)" | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the review body: this parameter,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787: |
||||||||||||||||||||||||||||||||||||||
| FingerprintFile="$(IntermediateOutputPath)protoc.fingerprint.txt"> | ||||||||||||||||||||||||||||||||||||||
|
pepone marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||
| <Output ItemName="_ProtoFile" TaskParameter="ComputedSources" /> | ||||||||||||||||||||||||||||||||||||||
| <Output PropertyName="_ProtocFingerprintChanged" TaskParameter="FingerprintChanged" /> | ||||||||||||||||||||||||||||||||||||||
| </UpToDateCheckTask> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <!-- | ||||||||||||||||||||||||||||||||||||||
| When the fingerprint changed, the recorded one describes outputs protoc is about to replace. Delete it first, | ||||||||||||||||||||||||||||||||||||||
| so a run that fails part-way leaves no fingerprint for the next build to trust; _ProtoWriteOutputManifest | ||||||||||||||||||||||||||||||||||||||
| records the new one once protoc succeeds. | ||||||||||||||||||||||||||||||||||||||
| --> | ||||||||||||||||||||||||||||||||||||||
| <Delete | ||||||||||||||||||||||||||||||||||||||
| Files="$(IntermediateOutputPath)protoc.fingerprint.txt" | ||||||||||||||||||||||||||||||||||||||
| Condition="'$(_ProtocFingerprintChanged)' == 'true' And Exists('$(IntermediateOutputPath)protoc.fingerprint.txt')" /> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <PropertyGroup> | ||||||||||||||||||||||||||||||||||||||
| <!-- Parameter for the Build Telemetry plug-in --> | ||||||||||||||||||||||||||||||||||||||
| <_BuildTelemetryParameter>toolchain=dotnet:$(NETCoreSdkVersion),plugin=csharp:$(ProtocBundledPluginVersion),plugin=icerpc-csharp:$(ProtocIceRpcPluginVersion),plugin=icerpc-build-telemetry:$(ProtocIceRpcPluginVersion)</_BuildTelemetryParameter> | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -121,18 +203,35 @@ | |||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||
| </ItemGroup> | ||||||||||||||||||||||||||||||||||||||
| </Target> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <!-- | ||||||||||||||||||||||||||||||||||||||
| _ProtoCleanFile must stay distinct from _ProtoFile: a task <Output> appends to an existing item, and Rebuild runs | ||||||||||||||||||||||||||||||||||||||
| Clean then Build in the same project instance. | ||||||||||||||||||||||||||||||||||||||
| AfterTargets keeps this after a successful protoc run, so a failed run never records a fingerprint its outputs | ||||||||||||||||||||||||||||||||||||||
| don't match (when the fingerprint changed, ProtoCompile deleted the previous one). The fingerprint is escaped | ||||||||||||||||||||||||||||||||||||||
| because WriteLinesToFile splits its Lines on semicolons. | ||||||||||||||||||||||||||||||||||||||
| --> | ||||||||||||||||||||||||||||||||||||||
| <Target Name="ProtoClean" BeforeTargets="Clean" Condition="Exists('$(IceRpcProtobufToolsTaskAssembliesPath)IceRpc.Protobuf.Tools.dll')"> | ||||||||||||||||||||||||||||||||||||||
| <OutputFileNamesTask Sources="@(ProtoFile)"> | ||||||||||||||||||||||||||||||||||||||
| <Output ItemName="_ProtoCleanFile" TaskParameter="ComputedSources" /> | ||||||||||||||||||||||||||||||||||||||
| </OutputFileNamesTask> | ||||||||||||||||||||||||||||||||||||||
| <Delete Files="@(_ProtoCleanFile->'%(OutputDir)/%(OutputFileName).cs')" /> | ||||||||||||||||||||||||||||||||||||||
| <Delete Files="@(_ProtoCleanFile->'%(OutputDir)/%(OutputFileName).IceRpc.cs')" /> | ||||||||||||||||||||||||||||||||||||||
| <Delete Files="@(_ProtoCleanFile->'%(OutputDir)/%(OutputFileName).d')" /> | ||||||||||||||||||||||||||||||||||||||
| <Delete Files="@(_ProtoCleanFile->'%(OutputDir)/%(OutputFileName).BuildTelemetry.txt')" /> | ||||||||||||||||||||||||||||||||||||||
| <Target Name="_ProtoWriteOutputManifest" | ||||||||||||||||||||||||||||||||||||||
| AfterTargets="ProtoCompile" | ||||||||||||||||||||||||||||||||||||||
| DependsOnTargets="_ComputeProtoOutputs" | ||||||||||||||||||||||||||||||||||||||
| Condition="@(ProtoFile) != ''"> | ||||||||||||||||||||||||||||||||||||||
| <MakeDir Directories="$(IntermediateOutputPath)" /> | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WriteLinesToFile creates the directory, and PrepareForBuild already created it. Drop the line.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787. |
||||||||||||||||||||||||||||||||||||||
| <WriteLinesToFile | ||||||||||||||||||||||||||||||||||||||
| File="$(IntermediateOutputPath)protoc.outputs.txt" | ||||||||||||||||||||||||||||||||||||||
| Lines="@(_ProtoOutput)" | ||||||||||||||||||||||||||||||||||||||
| Overwrite="true" | ||||||||||||||||||||||||||||||||||||||
| WriteOnlyWhenDifferent="true" /> | ||||||||||||||||||||||||||||||||||||||
| <WriteLinesToFile | ||||||||||||||||||||||||||||||||||||||
| File="$(IntermediateOutputPath)protoc.fingerprint.txt" | ||||||||||||||||||||||||||||||||||||||
| Lines="$([MSBuild]::Escape($(_ProtocFingerprint)))" | ||||||||||||||||||||||||||||||||||||||
| Overwrite="true" | ||||||||||||||||||||||||||||||||||||||
| WriteOnlyWhenDifferent="true" /> | ||||||||||||||||||||||||||||||||||||||
| </Target> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <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> | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+227
to
232
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787; the Delete now covers |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| <!-- Package ProtoFile items --> | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -75,9 +75,15 @@ once per Proto file. | |||||||||||||||||
| | Pack | `false` | Specifies whether or not to include the items (Proto files) in the NuGet package. | | ||||||||||||||||||
| | PackagePath | protobuf | Sets the target path in the NuGet package. Used only when Pack is `true`. | | ||||||||||||||||||
|
|
||||||||||||||||||
| > [!NOTE] | ||||||||||||||||||
| > Changing `AdditionalOptions` does not mark previously generated code as out of date. Run `dotnet clean` and then | ||||||||||||||||||
| > build again to regenerate the code with the new options. | ||||||||||||||||||
| ## Incremental builds | ||||||||||||||||||
|
|
||||||||||||||||||
| `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. | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787. |
||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787. |
||||||||||||||||||
|
|
||||||||||||||||||
| ## Generated code and NuGet packages | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,21 @@ namespace IceRpc.Protobuf.Tools; | |||||
| /// <summary>A MSBuild task to compute what Protobuf files have to be rebuild by <c>protoc</c>.</summary> | ||||||
| public class UpToDateCheckTask : Microsoft.Build.Utilities.Task | ||||||
| { | ||||||
| /// <summary>Gets or sets additional input files that every source depends on, typically the <c>protoc</c> | ||||||
| /// compiler and the code generator plug-in. A source is out of date when any of these files is missing or is | ||||||
| /// newer than one of the source's outputs.</summary> | ||||||
| public ITaskItem[] AdditionalInputs { get; set; } = []; | ||||||
|
|
||||||
| /// <summary>Gets or sets a string that identifies the configuration used to generate the outputs, such as the | ||||||
| /// compiler and plug-in versions and the options passed to them. When this value differs from the content of | ||||||
| /// <see cref="FingerprintFile"/>, every source is out of date.</summary> | ||||||
| public string Fingerprint { get; set; } = ""; | ||||||
|
|
||||||
| /// <summary>Gets or sets the path of the file that holds the <see cref="Fingerprint"/> recorded by the previous | ||||||
| /// successful build. A missing file counts as a changed fingerprint. When empty, the fingerprint check is | ||||||
| /// skipped.</summary> | ||||||
| public string FingerprintFile { get; set; } = ""; | ||||||
|
|
||||||
| /// <summary>Gets or sets the output directory for the generated code.</summary> | ||||||
| [Required] | ||||||
| public string OutputDir { get; set; } = ""; | ||||||
|
|
@@ -25,48 +40,52 @@ public class UpToDateCheckTask : Microsoft.Build.Utilities.Task | |||||
| [Output] | ||||||
| public ITaskItem[] ComputedSources { get; private set; } = []; | ||||||
|
|
||||||
| /// <summary>Gets a value indicating whether <see cref="Fingerprint"/> differs from the content of | ||||||
| /// <see cref="FingerprintFile"/>.</summary> | ||||||
| [Output] | ||||||
| public bool FingerprintChanged { get; private set; } | ||||||
|
|
||||||
| /// <summary>Computes whether or not an output file is up to date or needs to be rebuilt. After executing this | ||||||
| /// task, <see cref="ComputedSources"/> contains a task item for each item in <see cref="Sources"/> with two | ||||||
| /// additional metadata entries. The <c>UpToDate</c> metadata is set to 'true' or 'false', indicating whether the | ||||||
| /// item is up to date or needs to be rebuilt. The <c>OutputFileName</c> metadata contains the base file name for | ||||||
| /// the generated outputs. This is the input item's file name without the extension, and converted to PascalCase. | ||||||
| /// </summary> | ||||||
| /// <remarks>A source is up to date only when the <see cref="Fingerprint"/> matches the recorded one, all of its | ||||||
| /// outputs exist, every input recorded in its dependency file and every <see cref="AdditionalInputs"/> entry | ||||||
| /// exists, and the newest input is older than the oldest output.</remarks> | ||||||
| /// <returns>Returns <see langword="true"/> if the task was executed successfully, <see langword="false"/> | ||||||
| /// otherwise.</returns> | ||||||
| public override bool Execute() | ||||||
| { | ||||||
| var computedSources = new List<ITaskItem>(); | ||||||
| bool fingerprintChanged = false; | ||||||
| if (FingerprintFile.Length > 0) | ||||||
| { | ||||||
| fingerprintChanged = | ||||||
| !File.Exists(FingerprintFile) || File.ReadAllText(FingerprintFile).Trim() != Fingerprint.Trim(); | ||||||
| if (fingerprintChanged) | ||||||
| { | ||||||
| Log.LogMessage( | ||||||
| MessageImportance.Normal, | ||||||
| "The protoc configuration changed since the previous build; all Proto files are out of date."); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| string[] additionalInputs = [.. AdditionalInputs.Select(item => item.GetMetadata("FullPath"))]; | ||||||
|
|
||||||
| var computedSources = new List<ITaskItem>(); | ||||||
| foreach (ITaskItem source in Sources) | ||||||
| { | ||||||
| bool upToDate = true; | ||||||
| string fileName = source.GetMetadata("FileName").ToProtocPascalCase(); | ||||||
| string dependOutput = Path.Combine(OutputDir, $"{fileName}.d"); | ||||||
| string csharpOutput = Path.Combine(OutputDir, $"{fileName}.cs"); | ||||||
| string icerpcOutput = Path.Combine(OutputDir, $"{fileName}.IceRpc.cs"); | ||||||
| string[] outputs = | ||||||
| [ | ||||||
| dependOutput, | ||||||
| Path.Combine(OutputDir, $"{fileName}.cs"), | ||||||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||||||
| Path.Combine(OutputDir, $"{fileName}.IceRpc.cs"), | ||||||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||||||
| ]; | ||||||
|
|
||||||
| if (File.Exists(dependOutput) && File.Exists(csharpOutput) && File.Exists(icerpcOutput)) | ||||||
| { | ||||||
| long lastWriteTime = Math.Max( | ||||||
| File.GetLastWriteTime(dependOutput).Ticks, | ||||||
| File.GetLastWriteTime(csharpOutput).Ticks); | ||||||
| lastWriteTime = Math.Max(lastWriteTime, File.GetLastWriteTime(icerpcOutput).Ticks); | ||||||
| List<string> dependencies = ProcessDependencies(dependOutput); | ||||||
| foreach (string dependency in dependencies) | ||||||
| { | ||||||
| if (File.GetLastWriteTime(dependency).Ticks >= lastWriteTime) | ||||||
| { | ||||||
| // If a dependency is newer than any of the outputs the source is not up to date. | ||||||
| upToDate = false; | ||||||
| break; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| // If any of the outputs is missing the file is not up to date. | ||||||
| upToDate = false; | ||||||
| } | ||||||
| bool upToDate = !fingerprintChanged && IsUpToDate(source.ItemSpec, outputs, dependOutput, additionalInputs); | ||||||
|
|
||||||
| var computedSource = new TaskItem(source.ItemSpec); | ||||||
| source.CopyMetadataTo(computedSource); | ||||||
|
|
@@ -77,37 +96,71 @@ public override bool Execute() | |||||
| } | ||||||
|
|
||||||
| ComputedSources = [.. computedSources]; | ||||||
| FingerprintChanged = fingerprintChanged; | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| static List<string> ProcessDependencies(string dependOutput) | ||||||
| private bool IsUpToDate(string source, string[] outputs, string dependOutput, string[] additionalInputs) | ||||||
| { | ||||||
| string? missingOutput = outputs.FirstOrDefault(output => !File.Exists(output)); | ||||||
| if (missingOutput is not null) | ||||||
| { | ||||||
| var depends = new List<string>(); | ||||||
| string dependContents = File.ReadAllText(dependOutput); | ||||||
| Log.LogMessage(MessageImportance.Low, $"'{source}' is out of date: output '{missingOutput}' is missing."); | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| // Every output must be newer than every input, so compare against the oldest output. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second clause narrates the
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude: Done in e323787. |
||||||
| long oldestOutputTime = outputs.Min(output => File.GetLastWriteTime(output).Ticks); | ||||||
|
|
||||||
| // Strip everything before and including "Xxx.cs:" (the output target). | ||||||
| const string outputPrefix = ".cs:"; | ||||||
| int i = dependContents.IndexOf(outputPrefix, StringComparison.CurrentCultureIgnoreCase); | ||||||
| if (i == -1 || i + outputPrefix.Length >= dependContents.Length) | ||||||
| foreach (string input in ProcessDependencies(dependOutput).Concat(additionalInputs)) | ||||||
| { | ||||||
| if (!File.Exists(input)) | ||||||
| { | ||||||
| return depends; | ||||||
| // File.GetLastWriteTime returns a placeholder date for a missing file, older than any output. | ||||||
|
pepone marked this conversation as resolved.
Outdated
|
||||||
| Log.LogMessage(MessageImportance.Low, $"'{source}' is out of date: input '{input}' is missing."); | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| dependContents = dependContents[(i + outputPrefix.Length)..]; | ||||||
|
|
||||||
| // The Make depfile format uses '\' at end of line as a line continuation, and escapes | ||||||
| // spaces inside paths as '\ '. Windows directory separators are emitted as literal '\' | ||||||
| // (not escaped). We split on newlines, strip the trailing continuation '\' and whitespace, | ||||||
| // then unescape '\ ' -> ' ' so paths containing spaces resolve correctly. | ||||||
| foreach (string line in dependContents.Split('\n')) | ||||||
| if (File.GetLastWriteTime(input).Ticks >= oldestOutputTime) | ||||||
| { | ||||||
| string filePath = line.TrimEnd().TrimEnd('\\').Trim().Replace("\\ ", " ", StringComparison.Ordinal); | ||||||
| if (!string.IsNullOrEmpty(filePath)) | ||||||
| { | ||||||
| depends.Add(Path.GetFullPath(filePath)); | ||||||
| } | ||||||
| Log.LogMessage( | ||||||
| MessageImportance.Low, | ||||||
| $"'{source}' is out of date: input '{input}' is newer than one of its outputs."); | ||||||
| return false; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| private static List<string> ProcessDependencies(string dependOutput) | ||||||
| { | ||||||
| var depends = new List<string>(); | ||||||
| string dependContents = File.ReadAllText(dependOutput); | ||||||
|
|
||||||
| // Strip everything before and including "Xxx.cs:" (the output target). | ||||||
| const string outputPrefix = ".cs:"; | ||||||
| int i = dependContents.IndexOf(outputPrefix, StringComparison.CurrentCultureIgnoreCase); | ||||||
| if (i == -1 || i + outputPrefix.Length >= dependContents.Length) | ||||||
| { | ||||||
| return depends; | ||||||
| } | ||||||
|
|
||||||
| dependContents = dependContents[(i + outputPrefix.Length)..]; | ||||||
|
|
||||||
| // The Make depfile format uses '\' at end of line as a line continuation, and escapes | ||||||
| // spaces inside paths as '\ '. Windows directory separators are emitted as literal '\' | ||||||
| // (not escaped). We split on newlines, strip the trailing continuation '\' and whitespace, | ||||||
| // then unescape '\ ' -> ' ' so paths containing spaces resolve correctly. | ||||||
| foreach (string line in dependContents.Split('\n')) | ||||||
| { | ||||||
| string filePath = line.TrimEnd().TrimEnd('\\').Trim().Replace("\\ ", " ", StringComparison.Ordinal); | ||||||
| if (!string.IsNullOrEmpty(filePath)) | ||||||
| { | ||||||
| depends.Add(Path.GetFullPath(filePath)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return depends; | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
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.dcompared 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".
There was a problem hiding this comment.
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
AdditionalOptionsvalues (@(ProtoFile->'options=%(AdditionalOptions)'->DistinctWithCase()); with-case becauseDistinct()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.dwould 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to the per-file record. protoc runs once per Proto file, so the options are a per-file input, and the build
already keeps per-file state for every Proto file: its dependency file. Record the options next to it and the check
becomes exact instead of a set approximation. The cache file stays for the inputs that are global: the versions and
the search path.
It is smaller than it sounds:
_ComputeProtoOutputsgets one more_ProtoOutputInclude,%(OutputFileName).options(or a name you prefer).That one line covers the manifest, prune and clean.
ProtoCompilegets aWriteLinesToFileafterProtocTask, batched on%(_ProtoFile...)with the sameUpToDate != 'true'condition,Lines="%(_ProtoFile.AdditionalOptions)". Running after protoc, a failed runleaves no fresh record.
UpToDateCheckTaskadds the record tooutputsand compares its lines with theAdditionalOptionsmetadata italready gets on each source. Missing or different means out of date. No new parameter.
The
options=line, theDistinctWithCasetransform and its comment then leave the cache.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Claude: Done in 033a19c, with one change to the mechanism. The record is
<OutputDir>/<Name>.options, one option per line, listed by_ComputeProtoOutputsso the manifest, prune and clean cover it, and theoptions=line andDistinctWithCaseleft the cache. ButProtoCompilewrites it before the check withWriteOnlyWhenDifferent, and the task lists it among the file's inputs, the same rule asprotoc.inputs.cacheapplied per file. Two reasons, both reproduced with the after-protoc version first:%3Band, on Unix, for a backslash in an option value. With the record as an input there is nothing to compare.Verified with a two-file probe: one file's options changed / unchanged / reverted regenerates that file only (1 / 0 / 1); a file switching to the value the other uses, 1; a sibling failure with an options change regenerates only the failed file once fixed (1), and with the change reverted, the reverted file as well (2); third file added 1, removed 0 with
Third.optionspruned; task assembly missing, task-load error with the generated code intact; Clean removes the records.IceRpc.Protobuf.Tests: first build 5 (no records yet), second 0, 70 tests pass.