Skip to content

Use -Zpackage-workspace for packing #2349

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ members = [
]
exclude = [
"eng/scripts",
"target",
Copy link
Member

Choose a reason for hiding this comment

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

This is always ignored. Why did you add it? This is going to raise a lot of eyebrows.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not ignored 😬. The targets folder will get package folder in it. If you run any cargo commands on the packages in /targets, it complains about the package thinking its running in a workspace but not being a member or excluded

Copy link
Member

Choose a reason for hiding this comment

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

Why are you running commands against anything targets? That's not our place space. For package analysis - though I maintain there's nothing to analyze (it's zipped source) - that should probably be done outside the source repo in the temp working dir AzDO, GHA, etc. give us. Let's not unnecessarily complicate things.

]

[workspace.package]
Expand Down
4 changes: 0 additions & 4 deletions eng/pipelines/templates/jobs/pack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ jobs:
WorkingDirectory: $(System.DefaultWorkingDirectory)/eng/tools/generate_api_report
SetDefault: false

- template: /eng/pipelines/templates/steps/use-rust.yml@self
parameters:
Toolchain: stable

- ${{ if eq(parameters.TestPipeline, 'true') }}:
- template: /eng/common/pipelines/templates/steps/set-test-pipeline-version.yml
parameters:
Expand Down
23 changes: 19 additions & 4 deletions eng/pipelines/templates/stages/archetype-rust-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,30 @@ stages:
$additionalOwners = @('heaths', 'hallipr')
$token = $env:CARGO_REGISTRY_TOKEN
$crateName = '${{artifact.name}}'
$artifactPath = "$(Pipeline.Workspace)/drop/$crateName"

Write-Host "Dry-run verifying '$crateName'"

# remove Cargo.toml.orig before calling dry-run as it causes a reserved file name error
Remove-Item -Path "$artifactPath/contents/Cargo.toml.orig" -Force -ErrorAction Ignore

$command = "cargo publish --manifest-path '$artifactPath/contents/Cargo.toml' --dry-run"
Write-Host "> $command"
Invoke-Expression $command

$manifestPath = "$(Pipeline.Workspace)/drop/$crateName/contents/Cargo.toml"
Write-Host "> cargo publish --manifest-path `"$manifestPath`""
cargo publish --manifest-path $manifestPath
if (!$?) {
Write-Error "Failed to publish package: '$crateName'"
Write-Error "Dry-run failed for '$crateName'"
exit 1
}

Write-Host "Publishing package '$crateName'"

# https://doc.rust-lang.org/cargo/reference/registry-web-api.html#publish
Invoke-WebRequest -Method Put -Uri 'https://crates.io/api/v1/crates/new' `
-Headers @{ Accept = 'application/json'; Authorization = $token } `
-ContentType 'application/json' `
-InFile "$artifactPath/$crateName.bin"

$existingOwners = (cargo owner --list $crateName) -replace " \(.*", ""
$missingOwners = $additionalOwners | Where-Object { $existingOwners -notcontains $_ }

Expand Down
147 changes: 101 additions & 46 deletions eng/scripts/Pack-Crates.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ param(
[string[]]$PackageNames,
[Parameter(ParameterSetName = 'PackageInfo')]
[string]$PackageInfoDirectory,
[switch]$NoVerify
[switch]$Verify
)

$ErrorActionPreference = 'Stop'
Expand Down Expand Up @@ -80,8 +80,8 @@ function Get-CargoPackages() {
}

function Get-PackagesToBuild() {
$packages = Get-CargoPackages
$outputPackageNames = Get-OutputPackageNames $packages
[array]$packages = Get-CargoPackages
[array]$outputPackageNames = Get-OutputPackageNames $packages

# We start with output packages, then recursively add unreleased dependencies to the list of packages that need to be built
[array]$packagesToBuild = $packages | Where-Object { $outputPackageNames.Contains($_.name) }
Expand Down Expand Up @@ -126,26 +126,65 @@ function Get-PackagesToBuild() {
return $buildOrder
}

function Initialize-VendorDirectory() {
$path = "$RepoRoot/target/vendor"
Invoke-LoggedCommand "cargo vendor $path" -GroupOutput | Out-Host
return $path
}
# https://doc.rust-lang.org/cargo/reference/registry-web-api.html#publish
# https://github.com/rust-lang/cargo/blob/5c87c14f9a162daf10d4133fdaab35c72d67b018/crates/crates-io/lib.rs#L42
function Get-ApiMetadata($package) {
$packagePath = Split-Path -Path $package.manifest_path -Parent
$readmePath = $package.readme ? (Join-Path -Path $packagePath -ChildPath $package.readme) : $null

$jsonBody = [ordered]@{
'name' = $package.name
'vers' = $package.version
'deps' = @()
'features' = $package.features
'authors' = $package.authors
'description' = $package.description
'documentation' = $package.documentation
'homepage' = $package.homepage
'readme' = if ($readmePath -and (Test-Path -Path $readmePath)) {
Get-Content -Path $readmePath -Raw
}
else {
$null
}
'readme_file' = $package.readme
'keywords' = $package.keywords
'categories' = $package.categories
'license' = $package.license
'license_file' = $package.license_file
'repository' = $package.repository
'links' = $package.links
'rust_version' = $package.rust_version
}

function Add-CrateToLocalRegistry($LocalRegistryPath, $Package) {
$packageName = $Package.name
$packageVersion = $Package.version
foreach ($dependency in $package.dependencies) {
$jsonBody.deps += @{
'name' = $dependency.name
'version_req' = $dependency.req
'features' = $dependency.features
'optional' = $dependency.optional
'default_features' = $dependency.default_features
'target' = $dependency.target
'kind' = $dependency.kind
'explicit_name_in_toml' = $dependency.rename
}
}

# create an index entry for the package
$packagePath = "$RepoRoot/target/package/$packageName-$packageVersion"
return $jsonBody
}

Write-Host "Copying package '$packageName' to vendor directory '$LocalRegistryPath'"
Copy-Item -Path $packagePath -Destination $LocalRegistryPath -Recurse
function New-ApiPutFile($crateMetadata, $crateFilePath) {
$metadataBytes = [Text.Encoding]::Utf8.GetBytes($crateMetadata)
$metadataLengthBytes = [BitConverter]::GetBytes([UInt32]$metadataBytes.Length)
$crateBytes = [IO.File]::ReadAllBytes($crateFilePath)
$crateLengthBytes = [BitConverter]::GetBytes([UInt32]$crateBytes.Length)

#write an empty checksum file
'{"files":{}}' | Out-File -FilePath "$LocalRegistryPath/$packageName-$packageVersion/.cargo-checksum.json" -Encoding utf8
$bytes += $metadataLengthBytes + $metadataBytes + $crateLengthBytes + $crateBytes

return $bytes
}
Comment on lines +176 to 185
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use Get-FileHash? It's been around forever. Best not to reinvent the wheel and own its maintenance.

Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't computing a hash. Its constructing a binary file for posting to the crates.io api

Copy link
Member

Choose a reason for hiding this comment

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

...which comes back to my original question: why are we doing that? I get why we crack open .nupkg files: there's binaries to check for signing, viruses, etc. .crate files are zipped source. We spending so much time on redoing what cargo publish already does which not only prevents some other work from getting done, but I worry creates a dependency on undocumented implementation details we'll forever have to maintain ourselves.



function Create-ApiViewFile($package) {
$packageName = $package.name
$command = "cargo run --manifest-path $RepoRoot/eng/tools/generate_api_report/Cargo.toml -- --package $packageName"
Expand All @@ -158,61 +197,77 @@ function Create-ApiViewFile($package) {

Push-Location $RepoRoot
try {
$localRegistryPath = Initialize-VendorDirectory

[array]$packages = Get-PackagesToBuild

Write-Host "Building packages in the following order:"
$command = "cargo +nightly -Zpackage-workspace package --allow-dirty --locked"

Write-Host "Building packages:"
foreach ($package in $packages) {
$packageName = $package.name
$type = if ($package.OutputPackage) { "output" } else { "dependency" }
Write-Host " $packageName ($type)"
$command += " --package $packageName"
}

foreach ($package in $packages) {
Write-Host ""

$packageName = $package.name
$packageVersion = $package.version

$command = "cargo publish --locked --dry-run --package $packageName --registry crates-io --config `"source.crates-io.replace-with='local'`" --config `"source.local.directory='$localRegistryPath'`" --allow-dirty"
if (!$Verify) {
$command += " --no-verify"
}

if ($NoVerify) {
$command += " --no-verify"
}
if ($env:SYSTEM_DEBUG -eq 'true') {
Write-Host "##[group] $RepoRoot/Cargo.lock"
Get-Content "$RepoRoot/Cargo.lock"
Write-Host "##[endgroup]"
}

Invoke-LoggedCommand -Command $command -GroupOutput
Invoke-LoggedCommand -Command $command -GroupOutput

if ($env:SYSTEM_DEBUG -eq 'true') {
Write-Host "##[group] $RepoRoot/Cargo.lock"
Get-Content "$RepoRoot/Cargo.lock"
Write-Host "##[endgroup]"
}

# copy the package to the local registry
Add-CrateToLocalRegistry `
-LocalRegistryPath $localRegistryPath `
-Package $package
if ($OutputPath) {
foreach ($package in $packages | Where-Object { $_.OutputPackage }) {
$packageName = $package.name
$packageVersion = $package.version

Write-Host "`nProcessing package '$packageName'"

if ($OutputPath -and $package.OutputPackage) {
$sourcePath = "$RepoRoot/target/package/$packageName-$packageVersion"
$sourcePath = "$RepoRoot/target/package/$packageName-$packageVersion.crate"
$targetPath = "$OutputPath/$packageName"
$targetContentsPath = "$targetPath/contents"
$targetApiReviewFile = "$targetPath/$packageName.rust.json"
$targetCrateFile = "$targetPath/$packageName-$packageVersion.crate"
$targetJsonFile = "$targetPath/$packageName-$packageVersion.json"
$targetBinFile = "$targetPath/$packageName.bin"

if (Test-Path -Path $targetContentsPath) {
Remove-Item -Path $targetContentsPath -Recurse -Force
}

Write-Host "Copying package '$packageName' to '$targetContentsPath'"
Remove-Item -Path $targetPath -Recurse -Force -ErrorAction SilentlyContinue
New-Item -ItemType Directory -Path $targetContentsPath -Force | Out-Null
Copy-Item -Path $sourcePath/* -Destination $targetContentsPath -Recurse -Exclude "Cargo.toml.orig"

Write-Host "Copying crate file to '$targetCrateFile'"
Copy-Item -Path $sourcePath -Destination $targetCrateFile -Force

$crateMetadata = Get-ApiMetadata $package | ConvertTo-Json -Depth 10

Write-Host "Writing crates.io request metadata to '$targetJsonFile'"
$crateMetadata | Out-File -FilePath "$targetJsonFile" -Encoding utf8

$uploadBytes = New-ApiPutFile $crateMetadata $sourcePath
Write-Host "Writing crates.io request bundle to '$targetBinFile'"
[IO.File]::WriteAllBytes($targetBinFile, $uploadBytes)

Write-Host "Exctracting crate file to '$targetContentsPath'"
New-Item -ItemType Directory -Path $targetContentsPath -Force | Out-Null
tar -xf $sourcePath --directory $targetContentsPath --strip-components=1 | Out-Null

Write-Host "Creating API review file"
$apiReviewFile = Create-ApiViewFile $package

Write-Host "Copying API review file to '$targetApiReviewFile'"
Copy-Item -Path $apiReviewFile -Destination $targetApiReviewFile -Force
}
}

Write-Host "Removing local registry"
Remove-Item -Path $localRegistryPath -Recurse -Force | Out-Null
}
finally {
Pop-Location
Expand Down
2 changes: 1 addition & 1 deletion eng/scripts/Update-PackageVersion.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ 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
Expand Down
4 changes: 1 addition & 3 deletions eng/scripts/Yank-Crates.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,4 @@ foreach ($crateName in $crateNames) {
}
}

if ($hasErrors) {
exit 1
}
exit $hasErrors ? 1 : 0
31 changes: 21 additions & 10 deletions eng/scripts/update-pathversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
.expect("requires 'add' or 'update' mode argument");

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

// find all Cargo.toml files in the repo_root directory
let exclude_dirs = vec![
Expand All @@ -38,13 +38,13 @@ 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);
println!("Processing {}", toml_file.path.display());
update_package_versions(toml_file.document.as_table_mut(), &package_versions, None, should_add);

// if the toml file has a workspace table, update the workspace table
if let Some(workspace) = toml_file.document.get_mut("workspace") {
if let Some(table) = workspace.as_table_mut() {
update_package_versions(table, &package_versions, should_add);
update_package_versions(table, &package_versions, Some("workspace"), should_add);
}
}

Expand All @@ -57,6 +57,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
}

fn load_cargo_toml_files(repo_root: &PathBuf, exclude_dirs: &Vec<PathBuf>) -> Result<Vec<TomlInfo>, Box<dyn Error>> {
println!("Loading Cargo.toml files in {}", repo_root.display());
let mut toml_paths = Vec::new();
find_cargo_toml_files(repo_root, exclude_dirs, &mut toml_paths)?;

Expand Down Expand Up @@ -109,7 +110,7 @@ fn get_package_versions(toml_files: &Vec<TomlInfo>) -> Vec<(String, String, bool
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)>, prefix: Option<&str>, add: bool) {
// 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
Expand All @@ -121,16 +122,26 @@ fn update_package_versions(toml: &mut Table, package_versions: &Vec<(String, Str
let dependency_tables = get_dependency_tables(toml);

for (table_name, table) in dependency_tables {
let table_display_name = if prefix.is_some() {
format!("{}.{}", prefix.unwrap(), table_name)
} else {
table_name.clone()
};
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 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 dependency.get("path").is_some() {
if let Some(current_version) = dependency.get("version") {
if current_version.as_str() != Some(version) {
dependency["version"] = value(version);
println!(" Updating {table_display_name}.{package} to {version}");
}
} else if should_add {
dependency["version"] = value(version);
println!(" Adding {table_display_name}.{package} version {version}");
}
}
}
}
Expand Down