Skip to content

Commit fa94b68

Browse files
jevansaksCopilot
andauthored
Replace changelog system with automated API diff for PR review (#2273)
* Replace changelog system with automated API diff for PR review Remove the ChangesSinceLastRelease.txt system (which caused merge conflicts and lost history on every release) in favor of an automated approach: 1. WinmdUtils 'dump' command: Decompiles a winmd to sorted C# declarations using ICSharpCode.Decompiler, producing deterministic output suitable for text diffing. Shows full type declarations with attributes, method signatures, constants, and nested types. 2. Local diff script (scripts/DiffWinmdToBaseline.ps1): Dumps both the current build and last-release winmd, shows a unified diff via git diff. 3. CI artifact: The PR Validation workflow now publishes the winmd and its API dump as a 'winmd' artifact (retained 30 days). 4. PR API Diff workflow (.github/workflows/pr-api-diff.yml): Triggers after PR Validation succeeds, generates the baseline dump, diffs against the PR's dump, and posts/updates a comment on the PR with the full diff in a collapsible section. Removed: - scripts/ChangesSinceLastRelease.txt (no longer needed) - scripts/UpdateChangesSinceLastRelease.ps1 (no longer needed) - The build no longer fails on API diffs; reviewer sees them in PR comment Changed: - TestWinmdBinary.ps1: Compare is now informational (non-failing) - CompareBinToLastRelease.ps1: Simplified (no knownDiffs/update logic) - Set-LastReleaseVersion.ps1: Removed file-clearing logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Diff against main branch instead of last NuGet release The API diff now compares the PR's winmd against main's latest build artifact, showing only the changes introduced by the PR rather than all changes since the last release. Falls back to the NuGet release baseline if no main artifact is available (e.g., first run after enabling this feature). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Replace third-party action with actions/github-script for artifact download The microsoft org restricts actions to enterprise-owned or actions/* only. Use actions/github-script to call the GitHub API directly to download the baseline winmd artifact from the target branch's latest successful run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 1e9cdf1 commit fa94b68

14 files changed

Lines changed: 456 additions & 138 deletions

.github/copilot-instructions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ Each arch job scrapes ALL partitions for that architecture.
6666

6767
**NuGet feed:** The repo uses a private ADO Artifact Feed (`Win32Metadata-Dependencies`) as its sole package source. New packages from nuget.org must be saved to this feed first.
6868

69-
**Winmd equivalence:** After any change, the winmd must be compared against the baseline. The `NoSuggestedRemappings` test in `Windows.Win32.Tests` validates that no new unhandled remappings were introduced. Changes to the winmd are tracked in `scripts/ChangesSinceLastRelease.txt`.
69+
**Winmd equivalence:** After any change, the winmd must be compared against the baseline. The `NoSuggestedRemappings` test in `Windows.Win32.Tests` validates that no new unhandled remappings were introduced. API diffs are automatically posted as PR comments by CI. Run `.\scripts\DiffWinmdToBaseline.ps1` locally to see a full C# declaration diff against the last release.
7070

7171
**Cross-partition remaps:** Remaps in `scraper.settings.rsp` are global — they apply to all partitions. Auto-discovered remaps (from `Win32MetadataScraper`) are per-partition. If a tag name is used across partitions but the typedef is only in one partition's headers, the remap must be global (in `scraper.settings.rsp`) OR the referencing partition must `#include` the header with the typedef.
7272

.github/workflows/pr-validation.yml

Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ on:
2222
- 'docs/**'
2323
workflow_dispatch:
2424

25+
permissions:
26+
contents: read
27+
pull-requests: write
28+
2529
concurrency:
2630
group: pr-${{ github.event.pull_request.number || github.ref }}
2731
cancel-in-progress: true
@@ -135,6 +139,230 @@ jobs:
135139
shell: pwsh
136140
run: .\scripts\DoTests.ps1
137141

142+
- name: Download baseline winmd from target branch
143+
if: github.event_name == 'pull_request'
144+
id: baseline
145+
uses: actions/github-script@v7
146+
with:
147+
script: |
148+
const fs = require('fs');
149+
const path = require('path');
150+
151+
// Find the latest successful run on the target branch
152+
const baseBranch = context.payload.pull_request.base.ref;
153+
const runs = await github.rest.actions.listWorkflowRuns({
154+
owner: context.repo.owner,
155+
repo: context.repo.repo,
156+
workflow_id: 'pr-validation.yml',
157+
branch: baseBranch,
158+
status: 'success',
159+
per_page: 1
160+
});
161+
162+
if (runs.data.workflow_runs.length === 0) {
163+
core.warning(`No successful runs found on ${baseBranch}. Will fall back to NuGet baseline.`);
164+
return;
165+
}
166+
167+
const runId = runs.data.workflow_runs[0].id;
168+
core.info(`Found baseline run ${runId} on ${baseBranch}`);
169+
170+
// Find the winmd artifact
171+
const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
172+
owner: context.repo.owner,
173+
repo: context.repo.repo,
174+
run_id: runId
175+
});
176+
177+
const winmdArtifact = artifacts.data.artifacts.find(a => a.name === 'winmd');
178+
if (!winmdArtifact) {
179+
core.warning('No winmd artifact found in baseline run. Will fall back to NuGet baseline.');
180+
return;
181+
}
182+
183+
// Download and extract
184+
const download = await github.rest.actions.downloadArtifact({
185+
owner: context.repo.owner,
186+
repo: context.repo.repo,
187+
artifact_id: winmdArtifact.id,
188+
archive_format: 'zip'
189+
});
190+
191+
const outputDir = 'bin/baseline-artifact';
192+
fs.mkdirSync(outputDir, { recursive: true });
193+
const zipPath = path.join(outputDir, 'artifact.zip');
194+
fs.writeFileSync(zipPath, Buffer.from(download.data));
195+
196+
// Extract using PowerShell
197+
const { execSync } = require('child_process');
198+
execSync(`Expand-Archive -Path "${zipPath}" -DestinationPath "${outputDir}" -Force`, { shell: 'pwsh' });
199+
fs.unlinkSync(zipPath);
200+
core.info(`Extracted baseline winmd to ${outputDir}`);
201+
202+
- name: Generate API surface diff
203+
if: github.event_name == 'pull_request'
204+
id: apidump
205+
shell: pwsh
206+
run: |
207+
$ErrorActionPreference = 'Continue'
208+
$PSNativeCommandUseErrorActionPreference = $false
209+
210+
$winmdUtils = "bin\Release\net8.0\WinmdUtils.dll"
211+
$currentWinmd = "bin\Windows.Win32.winmd"
212+
213+
# Determine baseline: prefer main branch artifact, fall back to last release
214+
$baselineWinmd = "bin\baseline-artifact\Windows.Win32.winmd"
215+
if (-not (Test-Path $baselineWinmd)) {
216+
Write-Host "No main branch artifact found, falling back to last NuGet release..."
217+
. .\scripts\CommonUtils.ps1
218+
$baselineWinmd = Get-Win32MetadataLastReleaseWinmdPath
219+
}
220+
221+
Write-Host "Baseline: $baselineWinmd"
222+
Write-Host "Current: $currentWinmd"
223+
224+
# Dump baseline
225+
Write-Host "Dumping baseline..."
226+
& dotnet $winmdUtils dump --winmd $baselineWinmd --output bin\baseline.apidump.cs
227+
if ($LASTEXITCODE -ne 0) { throw "Failed to dump baseline" }
228+
229+
# Dump current build
230+
Write-Host "Dumping current build..."
231+
& dotnet $winmdUtils dump --winmd $currentWinmd --output bin\current.apidump.cs
232+
if ($LASTEXITCODE -ne 0) { throw "Failed to dump current" }
233+
234+
# Generate diff — git diff exits 1 when differences exist, which is expected
235+
& cmd /c "git diff --no-index --unified=3 bin\baseline.apidump.cs bin\current.apidump.cs > bin\api-diff.patch 2>&1"
236+
$diffExitCode = $LASTEXITCODE
237+
238+
if ($diffExitCode -eq 0) {
239+
Write-Host "No API differences found."
240+
"has_diff=false" | Out-File -FilePath $env:GITHUB_OUTPUT -Append
241+
} else {
242+
Write-Host "API differences detected."
243+
"has_diff=true" | Out-File -FilePath $env:GITHUB_OUTPUT -Append
244+
245+
# Count additions/deletions from the patch file
246+
$diffLines = Get-Content bin\api-diff.patch
247+
$additions = ($diffLines | Where-Object { $_ -match '^\+[^+]' }).Count
248+
$deletions = ($diffLines | Where-Object { $_ -match '^\-[^-]' }).Count
249+
"additions=$additions" | Out-File -FilePath $env:GITHUB_OUTPUT -Append
250+
"deletions=$deletions" | Out-File -FilePath $env:GITHUB_OUTPUT -Append
251+
252+
Write-Host "+$additions additions / -$deletions deletions"
253+
254+
# Truncate if too large for a GH comment
255+
$diffText = $diffLines -join "`n"
256+
$maxLen = 60000
257+
if ($diffText.Length -gt $maxLen) {
258+
$diffText = $diffText.Substring(0, $maxLen) + "`n`n... (diff truncated - see full artifact)"
259+
Set-Content -Path bin\api-diff.patch -Value $diffText -Encoding UTF8
260+
}
261+
}
262+
263+
# Reset LASTEXITCODE so GitHub Actions doesn't treat this step as failed
264+
$global:LASTEXITCODE = 0
265+
266+
- name: Post API diff comment on PR
267+
if: github.event_name == 'pull_request' && steps.apidump.outputs.has_diff == 'true'
268+
uses: actions/github-script@v7
269+
with:
270+
script: |
271+
const fs = require('fs');
272+
const diff = fs.readFileSync('bin/api-diff.patch', 'utf8');
273+
const prNumber = context.payload.pull_request.number;
274+
const additions = ${{ steps.apidump.outputs.additions || 0 }};
275+
const deletions = ${{ steps.apidump.outputs.deletions || 0 }};
276+
const runUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
277+
278+
const marker = '<!-- win32metadata-api-diff -->';
279+
const body = `${marker}
280+
## 📋 API Surface Diff
281+
282+
**+${additions} additions / -${deletions} deletions** vs main branch ([full build log](${runUrl}))
283+
284+
<details>
285+
<summary>Click to expand API diff</summary>
286+
287+
\`\`\`diff
288+
${diff}
289+
\`\`\`
290+
291+
</details>
292+
293+
> This comment is automatically updated on each push.`;
294+
295+
const comments = await github.rest.issues.listComments({
296+
owner: context.repo.owner,
297+
repo: context.repo.repo,
298+
issue_number: prNumber,
299+
per_page: 100
300+
});
301+
302+
const existing = comments.data.find(c => c.body.includes(marker));
303+
304+
if (existing) {
305+
await github.rest.issues.updateComment({
306+
owner: context.repo.owner,
307+
repo: context.repo.repo,
308+
comment_id: existing.id,
309+
body: body
310+
});
311+
} else {
312+
await github.rest.issues.createComment({
313+
owner: context.repo.owner,
314+
repo: context.repo.repo,
315+
issue_number: prNumber,
316+
body: body
317+
});
318+
}
319+
320+
- name: Post no-diff comment on PR
321+
if: github.event_name == 'pull_request' && steps.apidump.outputs.has_diff == 'false'
322+
uses: actions/github-script@v7
323+
with:
324+
script: |
325+
const prNumber = context.payload.pull_request.number;
326+
const marker = '<!-- win32metadata-api-diff -->';
327+
const body = `${marker}\n## 📋 API Surface Diff\n\n✅ No API differences vs last release.\n\n> This comment is automatically updated on each push.`;
328+
329+
const comments = await github.rest.issues.listComments({
330+
owner: context.repo.owner,
331+
repo: context.repo.repo,
332+
issue_number: prNumber,
333+
per_page: 100
334+
});
335+
336+
const existing = comments.data.find(c => c.body.includes(marker));
337+
338+
if (existing) {
339+
await github.rest.issues.updateComment({
340+
owner: context.repo.owner,
341+
repo: context.repo.repo,
342+
comment_id: existing.id,
343+
body: body
344+
});
345+
} else {
346+
await github.rest.issues.createComment({
347+
owner: context.repo.owner,
348+
repo: context.repo.repo,
349+
issue_number: prNumber,
350+
body: body
351+
});
352+
}
353+
354+
- name: Upload winmd and API dump
355+
uses: actions/upload-artifact@v4
356+
if: always()
357+
with:
358+
name: winmd
359+
path: |
360+
bin/Windows.Win32.winmd
361+
bin/current.apidump.cs
362+
bin/baseline.apidump.cs
363+
bin/api-diff.patch
364+
retention-days: 30
365+
138366
- name: Upload build logs
139367
uses: actions/upload-artifact@v4
140368
if: always()

CONTRIBUTING.md

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -252,20 +252,30 @@ Run `./DoAll.ps1 -ExcludePackages -ExcludeSamples` in [PowerShell 7](https://aka
252252

253253
Note that stale artifacts on your system may sometimes result in cryptic errors when attempting incremental builds. If you do encounter cryptic errors during incremental builds that you suspect are the result of previously built changes, reset your system state by running a clean build with `./DoAll.ps1 -Clean`.
254254

255-
### Comparing against the last release
255+
### Reviewing API changes
256256

257-
A list of accumulated changes since the last release is kept at [ChangesSinceLastRelease.txt](scripts/ChangesSinceLastRelease.txt). New changes are reported by `./scripts/TestWinmdBinary.ps1` which is called during both full and incremental builds if you follow the steps above.
257+
API differences between your build and the last release are automatically detected during the build. When you submit a PR, the CI pipeline posts a comment showing the full API diff as decompiled C# declarations, making it easy for reviewers to see exactly what changed.
258258

259-
When validating changes, it's important to evaluate the diffs to ensure all changes are intentional. Common patterns to expect in the diffs include:
259+
#### Viewing changes locally
260+
261+
To see a full diff of your changes locally before submitting a PR:
262+
263+
```powershell
264+
.\scripts\DiffWinmdToBaseline.ps1
265+
```
266+
267+
This decompiles both the current build's winmd and the last released winmd to sorted C# declarations, then displays a unified diff. The output shows full type declarations with attributes, method signatures, and constants — similar to what you'd see in ILSpy.
268+
269+
#### What to look for
270+
271+
When validating changes, it's important to evaluate the diffs to ensure all changes are intentional. Common patterns to expect include:
260272

261273
* APIs were added to the baseline
262274
* APIs were removed from the baseline
263275
* APIs were moved to different namespaces
264276

265277
Additionally, it is useful to load the winmd in [ILSpy](https://github.com/icsharpcode/ILSpy) and navigate through the APIs as another means to identify additional changes that may be required to achieve the desired end result. You may notice that two related APIs are in different namespaces or that a type that an API depends on was not moved as you would have expected. If that happens, search the repo for the API or its header file to identify where it may be being mapped to another namespace.
266278

267-
Once all the changes are validated, update the list of known changes since the last release by following the steps reported in the build output. When a new release is made, the list of changes in [ChangesSinceLastRelease.txt](scripts/ChangesSinceLastRelease.txt) will get reset and will start accumulating again until the next release.
268-
269279
## Releasing
270280

271281
The main branch must have a clean build to publish a new release. Run the [release pipeline](https://github-private.visualstudio.com/microsoft/_build?definitionId=750) to publish new packages to nuget.org and create a new draft release on GitHub autopopulated with the list of resolved issues.

docs/copilot/research/win32metadata-detailed-research.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ DoAll.ps1
475475

476476
6. **Allow-list testing pattern**: Tests use `.rsp` allow-list files to track known acceptable violations. This enables progressive quality improvement without blocking builds.
477477

478-
7. **Winmd diff as release gate**: Every build compares the new .winmd against the last release via `WinmdUtils compare`. The diff is tracked in `ChangesSinceLastRelease.txt` to ensure all changes are intentional.
478+
7. **Winmd diff for PR review**: Every build compares the new .winmd against the last release via `WinmdUtils compare`. The full API diff (decompiled C# declarations) is posted as a PR comment by CI for reviewer visibility.
479479

480480
8. **Separate pipelines for metadata vs docs**: The API documentation pipeline (`azure-pipelines-apidocs.yml`) is completely independent, triggered only by changes in `apidocs/`.
481481

docs/copilot/research/win32metadata-research-summary.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ Tasks are loaded via `UsingTask AssemblyFile` (in-process), but net8.0 assemblie
123123
- No full winmd binary snapshots (only ~100 interfaces)
124124
- No cross-build consistency tests ("build A vs build B")
125125
- No hash-based regression detection
126-
- `ChangesSinceLastRelease.txt` is manually maintained (400+ lines)
126+
- API diffs are posted as PR comments (full C# declarations via `WinmdUtils dump`)
127127

128128
### 3.5 Metadata Defined After-the-Fact
129129

0 commit comments

Comments
 (0)