Conversation
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
hittyt
left a comment
hittyt
left a comment
There was a problem hiding this comment.
Refined packaging for C# SDKs and updated project URLs. However, there is a critical issue in the GitHub workflow regarding the consistency of the project state between restore and pack steps, which will likely break the release pipeline.
| - name: Pack | ||
| if: startsWith(github.ref, format('refs/tags/csharp/{0}/v', matrix.sdk.tagPrefix)) | ||
| run: | | ||
| EXTRA_PACK_ARGS="" |
There was a problem hiding this comment.
[P1] Inconsistent project state between Restore and Pack steps
The dotnet pack command is now called with -p:UseLocalOpenSandboxProjectReference=false and --no-restore for the code-interpreter SDK. However, the preceding Restore step (line 58) does not receive this property, meaning it will restore the project using the default project references. When dotnet pack later attempts to build using package references with --no-restore, it will fail because the required package assets were never resolved or restored.
This property MUST be passed to both the Restore and Pack steps. A cleaner approach would be to add this to the matrix definition or set a shared environment variable.
There was a problem hiding this comment.
FIXED, Restore and Pack unified
| Default to local source reference for development. | ||
| For release packing with NuGet version range, run: | ||
| dotnet pack -c Release -p:UseLocalOpenSandboxProjectReference=false | ||
| --> |
There was a problem hiding this comment.
[P2] Hardcoded dependency version range
The version range [0.1.0,0.2.0) is hardcoded. In a monorepo, this manually maintained value is prone to diverging from the actual version of the Alibaba.OpenSandbox project.
If the goal is to enforce SemVer constraints in the generated package, consider using a shared MSBuild property or leveraging ProjectReference with specific PackageVersion metadata to automate this, ensuring the SDKs stay in sync without manual updates.
There was a problem hiding this comment.
Refined with shared MsBuild
ninan-nn
force-pushed
the
feature/refine_pkging
branch
from
5517bec to
46eb299
Compare
ninan-nn
force-pushed
the
feature/refine_pkging
branch
from
46eb299 to
17f5564
Compare
4 participants
Summary
Testing
Breaking Changes
Checklist