Output debug lines from update-pathversions.rs #3227
Conversation
hallipr
requested review from
RickWinter,
heaths and
weshaggard
as code owners
There was a problem hiding this comment.
Pull Request Overview
Adds debug/log output to the Rust script that updates dependency path versions so release pipeline issues are easier to diagnose. Key changes introduce println! tracing, make update_package_versions report whether it mutated a file, and surface script output in the PowerShell wrapper by removing Out-Null.
- Added logging throughout update-pathversions.rs (mode, files scanned, per-dependency updates).
- Changed update_package_versions to return a bool indicating whether changes occurred, and gated file writes on that.
- PowerShell script now shows cargo script and cargo update output (removed Out-Null) to expose logs.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| eng/scripts/update-pathversions.rs | Added logging, changed function return type, but introduced multiple syntax and structural errors. |
| eng/scripts/Update-PackageVersion.ps1 | Exposes output from the Rust helper by removing Out-Null. |
eng/scripts/update-pathversions.rs
Outdated
| if let Some(t | ||
| able) = workspace.as | ||
| _table_mut() {, |
There was a problem hiding this comment.
I agree with what Copilot has been saying to this point, but this comment is misleading. Clearly this should be in one line.
There's a lot wrong with the formatting of this script. I recommend removing the header (lsp support will land soon, it looks like, so you don't need to) and get rust-analyzer to format this, check for obvious errors, etc., before putting the header back in.
There was a problem hiding this comment.
I think there was a race between me saving the file and copilot updating it given our global setting of "editor.formatOnSave": true,
hallipr
force-pushed
the
users/pahallis/log-version-updates
branch
3 times, most recently
from
df2dd1e to
58601cf
Compare
hallipr
force-pushed
the
users/pahallis/log-version-updates
branch
from
58601cf to
dfff29f
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| let should_add = add | ||
| && table_name != "dev-dependencies" | ||
| && !is_publish_disabled | ||
| && package != "azure_identity"; |
There was a problem hiding this comment.
I wonder if we should even do this check. It should only ever be a dev-dependency anyway.
That said, do we make sure we only process Cargo.tomls that are part of the workspace? We have a few test projects that must use a path dependency but don't get published. They use crates as dependencies but will also never published which, I suppose, would be caught by the line above.
There was a problem hiding this comment.
We need to see when we would call the script with --add
When do we want to inject versions into path dependencies that didn't already have them. I'm remembering 'add' mode existing so we could have path-only dependencies in the repo that get converted to path + version dependencies in ci.
If that's the case, then it doesn't really matter where all we do that. We'll be adding a version requirement to a dependency when we're guaranteed that the version at the path matches the required version and we wouldn't commit the change.
| ); | ||
| updated = true; | ||
| } | ||
| } else if should_add { |
There was a problem hiding this comment.
Post-GA, shouldn't we remove the version or is the assumption here that the service crate devs would've already had to do that after azure_core, for example, shipped the week before?
There was a problem hiding this comment.
I think we need to spec out our dependency version scheme. I can't remember what we've said.
There was a problem hiding this comment.
From what I recall, if someone wants to depend on the yet-to-be-released version of azure_core, they'll take a path-only dependency, and we'll make that cargo packable by using update-packageversions add. Once azure_core is released, I believe our post-release PR will pin those dependencies to the version we just released.
We should decide of we want to change local path dependencies to pinned workspace dependencies.
If they want to go back to an unreleased path dependency, they'll do that manually
There was a problem hiding this comment.
I think we need to spec out our dependency version scheme. I can't remember what we've said.
Hence the old issue, #2475
There was a problem hiding this comment.
Once azure_core is released, I believe our post-release PR will pin those dependencies to the version we just released.
Too presumptuous. The crate only needs to once they are preparing to release, so we should have checks then. You're assuming they were waiting for the next azure_core but may actually be a long-lead issue.
Add version update logs to update-pathversions.rs to assist in debugging version update errors in our release pipelines