Skip to content

Output debug lines from update-pathversions.rs #3227

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+105 −23
Open

Output debug lines from update-pathversions.rs #3227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfff29f
Output debug lines from update-pathversions.rs
hallipr Oct 17, 2025
e08dbb0
Update eng/scripts/update-pathversions.rs
hallipr Oct 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions eng/scripts/Update-PackageVersion.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ if ($content -ne $updated) {
Write-Host "Updated version in $tomlPath from $($pkgProperties.Version) to $packageSemVer."

Write-Host "Updaging dependencies in Cargo.toml files."
Invoke-LoggedCommand "cargo +nightly -Zscript '$RepoRoot/eng/scripts/update-pathversions.rs' update" | Out-Null
Invoke-LoggedCommand "cargo +nightly -Zscript '$RepoRoot/eng/scripts/update-pathversions.rs' update"

Write-Host "Updating Cargo.lock using 'cargo update --workspace'."
Invoke-LoggedCommand "cargo update --workspace" | Out-Null
Invoke-LoggedCommand "cargo update --workspace"
}
else {
Write-Host "$tomlPath already contains version $packageSemVer"
Expand Down
124 changes: 103 additions & 21 deletions eng/scripts/update-pathversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,30 @@ use std::{env, error::Error, fs, path::PathBuf};
use toml_edit::{value, DocumentMut, Item, Table};

fn main() -> Result<(), Box<dyn std::error::Error>> {
let add_mode = env::args().nth(1)
let add_mode = env::args()
.nth(1)
.map(|arg| match arg.as_str() {
"add" => true,
"update" => false,
_ => panic!("Invalid mode. Use 'add' or 'update'.")
_ => panic!("Invalid mode. Use 'add' or 'update'."),
})
.expect("requires 'add' or 'update' mode argument");

println!(
"Running update-pathversions in {} mode",
if add_mode { "add" } else { "update" }
);

let script_root = PathBuf::from(env::var("CARGO_MANIFEST_DIR")?);
let repo_root = script_root.join("../../..").canonicalize()?;
let repo_root = script_root.join("../..").canonicalize()?;

println!(
"Scanning for Cargo.toml files under {}",
repo_root.display()
);

// find all Cargo.toml files in the repo_root directory
let exclude_dirs = vec![
repo_root.join("eng"),
repo_root.join("target")
];
let exclude_dirs = vec![repo_root.join("eng"), repo_root.join("target")];

let toml_files = load_cargo_toml_files(&repo_root, &exclude_dirs)?;

Expand All @@ -39,15 +47,26 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
for mut toml_file in toml_files {
let should_add = add_mode && !toml_file.is_publish_disabled;

update_package_versions(toml_file.document.as_table_mut(), &package_versions, should_add);
let mut updated = update_package_versions(
toml_file.document.as_table_mut(),
&package_versions,
should_add,
);

// if the toml file has a workspace table, update the workspace table
if let Some(workspace) = toml_file.document.get_mut("workspace") {
// print out that we're working on a workspace
if let Some(table) = workspace.as_table_mut() {
update_package_versions(table, &package_versions, should_add);
updated = update_package_versions(table, &package_versions, should_add) || updated;
}
}

if !updated {
continue;
}

println!("Updating {}", toml_file.path.display());

// write the updated document back to the file
let mut file = fs::File::create(toml_file.path)?;
fs::File::write_all(&mut file, toml_file.document.to_string().as_bytes())?;
Expand All @@ -56,32 +75,49 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
Ok(())
}

fn load_cargo_toml_files(repo_root: &PathBuf, exclude_dirs: &Vec<PathBuf>) -> Result<Vec<TomlInfo>, Box<dyn Error>> {
fn load_cargo_toml_files(
repo_root: &PathBuf,
exclude_dirs: &Vec<PathBuf>,
) -> Result<Vec<TomlInfo>, Box<dyn Error>> {
let mut toml_paths = Vec::new();
find_cargo_toml_files(repo_root, exclude_dirs, &mut toml_paths)?;

let mut toml_files = Vec::new();

for path in toml_paths {
let content = fs::read_to_string(&path)?;
let doc = content.parse::<DocumentMut>()?;
let package_table = doc.get("package").and_then(Item::as_table);
let publish_property = package_table.and_then(|table| table.get("publish")).and_then(Item::as_bool);
let package_name = package_table.and_then(|table| table.get("name")).and_then(Item::as_str);
let package_version = package_table.and_then(|table| table.get("version")).and_then(Item::as_str);

let publish_property = package_table
.and_then(|table| table.get("publish"))
.and_then(Item::as_bool);

let package_name = package_table
.and_then(|table| table.get("name"))
.and_then(Item::as_str);

let package_version = package_table
.and_then(|table| table.get("version"))
.and_then(Item::as_str);

toml_files.push(TomlInfo {
path,
package_name: package_name.map(|s| s.to_string()),
package_version: package_version.map(|s| s.to_string()),
is_publish_disabled: publish_property == Some(false),
document: doc
document: doc,
});
}

Ok(toml_files)
}

fn find_cargo_toml_files(dir: &PathBuf, exclude_dirs: &Vec<PathBuf>, toml_paths: &mut Vec<PathBuf>) -> Result<(), Box<dyn Error>> {
fn find_cargo_toml_files(
dir: &PathBuf,
exclude_dirs: &Vec<PathBuf>,
toml_paths: &mut Vec<PathBuf>,
) -> Result<(), Box<dyn Error>> {
for entry in fs::read_dir(dir)? {
let entry = entry?;
let path = entry.path();
Expand All @@ -103,38 +139,84 @@ fn get_package_versions(toml_files: &Vec<TomlInfo>) -> Vec<(String, String, bool
continue;
}

package_versions.push((toml_file.package_name.clone().unwrap(), toml_file.package_version.clone().unwrap(), toml_file.is_publish_disabled));
package_versions.push((
toml_file.package_name.clone().unwrap(),
toml_file.package_version.clone().unwrap(),
toml_file.is_publish_disabled,
));
}

package_versions
}

fn update_package_versions(toml: &mut Table, package_versions: &Vec<(String, String, bool)>, add: bool) {
fn update_package_versions(
toml: &mut Table,
package_versions: &Vec<(String, String, bool)>,
add: bool,
) -> bool {
// Returns `true` if any modifications were made to the TOML table, `false` otherwise.
// This indicates whether the file needs to be written back to disk.
//
// for each dependency table, for each package in package_versions
// if the package is in the dependency table
// if the dependency has both path and version properties, update the version property
// if the dependency has has path, but not version, add the version property only if
// 1. the table name is not "dev-dependencies"
// 2. the package is not publish disabled
// 3. the add flag is true
//
let crate_name = toml
.get("package")
.and_then(Item::as_table)
.and_then(|table| table.get("name"))
.and_then(Item::as_str)
.unwrap_or("<workspace>")
.trim_matches('"')
.trim()
.to_string();

let dependency_tables = get_dependency_tables(toml);

let mut updated = false;
for (table_name, table) in dependency_tables {
for (package, version, is_publish_disabled) in package_versions {
if let Some(dependency) = table.get_mut(package) {
// azure_idenentity will only be a transitive dev-dependency
let should_add = add && table_name != "dev-dependencies" && !is_publish_disabled && package != "azure_identity";
let should_add = add
&& table_name != "dev-dependencies"
&& !is_publish_disabled
&& package != "azure_identity";
Copy link
Member

@heaths heaths Oct 21, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@hallipr hallipr Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.


let has_path_property = dependency.get("path").is_some();
let has_version_property = dependency.get("version").is_some();

if has_path_property && (has_version_property || should_add) {
dependency["version"] = value(version);
if has_path_property {
if has_version_property {
let current_version = dependency
.get("version")
.and_then(Item::as_str)
.unwrap_or("");
if current_version != version {
dependency["version"] = value(version);
println!(
"set {} to version {} in {} {}",
package, version, crate_name, table_name
);
updated = true;
}
} else if should_add {
Copy link
Member

@heaths heaths Oct 21, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@hallipr hallipr Oct 29, 2025

Choose a reason for hiding this comment

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

I think we need to spec out our dependency version scheme. I can't remember what we've said.

Copy link
Member Author

@hallipr hallipr Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@heaths heaths Oct 30, 2025

Choose a reason for hiding this comment

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

I think we need to spec out our dependency version scheme. I can't remember what we've said.

Hence the old issue, #2475

Copy link
Member

@heaths heaths Oct 30, 2025

Choose a reason for hiding this comment

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

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.

dependency["version"] = value(version);
println!(
"added version {} to {} in {} {}",
version, package, crate_name, table_name
);
updated = true;
}
}
}
}
}

updated
}

fn get_dependency_tables(toml: &mut Table) -> Vec<(String, &mut Table)> {
Expand Down