fix: remove Vcpkg dependency from ProtoCompile - #1728
Conversation
Drop the DependsOnTargets="VcpkgInstallManifestDependencies" attribute from the ProtoCompile target in vc18/otclient.vcxproj. This allows the ProtoCompile step to run without forcing the vcpkg install manifest target to execute, avoiding build ordering issues when vcpkg integration is not required or present.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes an explicit dependency declaration from the MSBuild ChangesProto Build Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request modifies the MSBuild project file vc18/otclient.vcxproj to remove the DependsOnTargets="VcpkgInstallManifestDependencies" attribute from the ProtoCompile target. The reviewer noted that removing this dependency entirely can cause build failures in environments using vcpkg, as the protobuf compiler (protoc.exe) might not yet be restored when the target executes. A conditional dependency approach was suggested to support both vcpkg and non-vcpkg environments safely.
| <ProtoFiles Include="$(ProtoPath)\*.proto" /> | ||
| </ItemGroup> | ||
| <Target Name="ProtoCompile" BeforeTargets="PrepareForBuild" DependsOnTargets="VcpkgInstallManifestDependencies" Condition="'$(RunProtoCompile)'=='true'"> | ||
| <Target Name="ProtoCompile" BeforeTargets="PrepareForBuild" Condition="'$(RunProtoCompile)'=='true'"> |
There was a problem hiding this comment.
Removing DependsOnTargets="VcpkgInstallManifestDependencies" entirely can cause build failures when vcpkg is used and RunProtoCompile is true. Since ProtoCompile runs BeforeTargets="PrepareForBuild", it may execute before vcpkg has restored the dependencies, meaning protoc.exe will not yet exist in the vcpkg_installed directory.
To safely support environments both with and without vcpkg integration, we can conditionally define the dependency using an MSBuild property that checks if the vcpkg targets file exists.
<PropertyGroup>
<ProtoCompileDependsOn Condition="Exists('$(VcpkgRoot)\\scripts\\buildsystems\\msbuild\\vcpkg.targets')">VcpkgInstallManifestDependencies</ProtoCompileDependsOn>
</PropertyGroup>
<Target Name="ProtoCompile" BeforeTargets="PrepareForBuild" DependsOnTargets="$(ProtoCompileDependsOn)" Condition="'$(RunProtoCompile)'=='true'">
There was a problem hiding this comment.
Pull request overview
This PR updates the Visual Studio project’s protobuf generation step to no longer force execution of vcpkg’s manifest install target before running protoc, aiming to avoid build ordering issues when vcpkg MSBuild integration isn’t available.
Changes:
- Removes
DependsOnTargets="VcpkgInstallManifestDependencies"from theProtoCompileMSBuild target invc18/otclient.vcxproj.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Target Name="ProtoCompile" BeforeTargets="PrepareForBuild" Condition="'$(RunProtoCompile)'=='true'"> | ||
| <Exec Command=""$(ProtocPath)" --proto_path="$(ProtoPath)" --cpp_out="generated" "%(ProtoFiles.Identity)"" /> |
Drop the DependsOnTargets="VcpkgInstallManifestDependencies" attribute from the ProtoCompile target in vc18/otclient.vcxproj. This allows the ProtoCompile step to run without forcing the vcpkg install manifest target to execute, avoiding build ordering issues when vcpkg integration is not required or present.
Summary by CodeRabbit