-
Notifications
You must be signed in to change notification settings - Fork 57
Add support for uploading symbols to a public symbol server #1064
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
base: main
Are you sure you want to change the base?
Conversation
compnerd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to upload the symbols to both the public and private copies of the PDBs? Why not only retain the public (R2) and consider decommissioning the Azure storage?
| shell: pwsh | ||
| run: | | ||
| Write-Output "ℹ️ Downloading Windows SDK installer..." | ||
| Invoke-WebRequest -Uri 'https://go.microsoft.com/fwlink/?linkid=2120843' -OutFile winsdksetup.exe -UseBasicParsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a documented ever green URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not any documention about it how long this is going to be suported. this has been around for a while though (2004) so doubt it will go away. will add a line to document the version.
| "${env:ProgramFiles(x86)}\Windows Kits\10\Debuggers\x64\symstore.exe", | ||
| "${env:ProgramFiles(x86)}\Windows Kits\10\Debuggers\x86\symstore.exe", | ||
| "${env:ProgramFiles(x86)}\Windows Kits\10\Debuggers\arm64\symstore.exe" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this path guaranteed? Why not just query it programmatically? If you want a reference, see https://github.com/compnerd/swift-devenv/blob/main/Sources/devenv/main.swift#L32-L57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. we install the windows Kits explicitly the previous step. so it needs to be there if the installation succeeds. This is just was easier than looking up the curent arch, and having a hard coded path. same result though.
Not really. wanted to get some milage on this first, then we can switch. |
| } | ||
|
|
||
| Write-Output "ℹ️ Found $($PdbFiles.Count) PDB file(s) to add to symbol store" | ||
| $Product = "Swift-Toolchain - $env:ARCH" + ($(if ($env:VARIANT) { " ($env:VARIANT)" } else { "" })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this variable used anywhere?
|
|
||
| if (!$LASTEXITCODE) { | ||
| Write-Output "✓ server.txt uploaded successfully (no concurrent modification)" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: whitespace on empty line.
| if ($LASTEXITCODE) { | ||
| Write-Output "::error::Failed to get ETag of server.txt" | ||
| Write-Output $HeadOutput | ||
| exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop is set up to do retries. However, almost every failure ends with exit 1. Did you mean to continue instead?
| } | ||
|
|
||
| # Handle concurrency / retry | ||
| if ($UploadOutput -match "PreconditionFailed" -or $UploadOutput -match "412") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this error condition happen with the aws s3api put-object command on line 180 returning 0 to indicate success?
If not, this is unreachable.
This change adds support to uploading symbols to a public symbol server hosted on R2. To get access to the symbols, you need to add the public swift-toolchain to your sym path
_NT_SYMBOL_PATHor simply add it windbg while debugging:Details:
We are using Symstore.exe to prepare the symbols and manage their state. This tool is ment to be used on a single build machine or with a network share. Since we are using R2, and R2/S3 do not implement locking guarantees this opens us up to concurrent access to the store. The good news is that there is little chance of symbols collision since they are stored under a hash of the pdb file, but there are 3 text files that symstore.exe uses to record state of the symbols. these files are used to enable pruning symbols from the server when needed.
The implementation here uses an optimistic concurrency model where each worker will attempt to grab the latest state and then update it. We use the S3/R2 atomic operation
if-matchto ensure that no state change happened in between, if so we also upload the symbols, if not we retry.The process of preparing the symbols is rather short, this whole action adds 2-3 minutes for each build job, including the installation of the tools, and the actual upload.
Downstream runs: