Skip to content

Keep reference gathering framework-dependent - #3312

Open
LoopedBard3 wants to merge 4 commits into
dotnet:masterfrom
LoopedBard3:loopedbard3-r2r-reference-gathering
Open

LoopedBard3 wants to merge 4 commits into
dotnet:masterfrom
LoopedBard3:loopedbard3-r2r-reference-gathering

Conversation

@LoopedBard3

@LoopedBard3 LoopedBard3 commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Summary

Make the reference-gathering pass framework-dependent without changing the final toolchain deployment mode.

The generated R2R project sets SelfContained=true, while the shared gathering group sets UseAppHost=false. The SDK rejects that combination with NETSDK1067 before compilation. Mono has the same conflicting declarations.

Add SelfContained=false to the existing property group conditioned on BenchmarkDotNetGatherReferences=true. This pass only resolves references: it already skips compiler execution, apphost generation and copying the build product. Final R2R/Mono publishing still runs without the gather property and retains self-contained deployment. Final R2R/composite flags, RID and custom runtime/crossgen2 pack overrides are unchanged.

The contradiction exists in both the source pinned by 0.16.0-nightly.20261029.662 (7d72eaef1caeb1b511250368bcfcafc0cdd650bf) and the upstream base used here (f137abb1966172d0266a5b2c52952e87594a9c7f): R2RBuilder is identical between them, and CsProjBuilder differs only in unrelated comment indentation. This does not establish when the bug was introduced or attribute all consuming CI failures to it.

Changes

  • Add the gather-only SelfContained=false override in CsProjBuilder.
  • Exclude publishing CLI builders from ForcedNoDependenciesForIntegrationTests, as suggested in the review. The existing non-publishing CLI shortcut, dynamic-build exceptions and integration/.NET Core guards remain intact.
  • Expose the existing protected virtual publishing decision through an internal read-only query, without widening the public/protected override API or duplicating a list of toolchain types.
  • Keep R2RToolchainGathersReferencesBeforePublishing in the existing integration suite, now using the ordinary default job with no explicit Release workaround. Both R2R cases take the shared full-process path.
  • Check nonempty gathered reference files and usable expected assembly references before final composite-publish/execution assertions, so a warning-only gather failure cannot pass just because the benchmark eventually executes.
  • Add focused selection cases for non-publishing CoreCLR, R2R, Mono, NativeAOT, Mono/CoreCLR Wasm, CoreRun, in-process builders and dynamic-build characteristics. These inspect selection without requiring or executing optional workloads.
  • Keep the new SDK-only integration case enabled on draft PRs; retain its .NET Core requirement and leave the existing R2R execution test in place.

The generated-property theory was removed in response to review, along with its helper scaffolding and the test-only CreateProject accessibility change. No reference filtering, gather-failure reporting, target frameworks, dependency versions or final publishing settings are changed. Publishing integration cases now use the existing full dependency/artifact path rather than its no-dependencies variant; normal benchmark runs are unaffected.

Validation

An independent Claude Sonnet 5.5 pre-implementation design review approved the gather-only override. Separate narrow pre-edit reviews covered the integration design, cross-platform assertion repair and this broader shared publishing-selection change.

  • A controlled full-process comparison used the identical integration test in the fixed worktree and an owned snapshot with only the gather override removed. Under the previous behavior, restore succeeded, the gather build failed with NETSDK1067, and neither ref.txt nor ref.props existed. Final publish and execution still succeeded, demonstrating why execution-only coverage missed the bug.
  • With the fix, actual generated-project restore, gathering, usable expected references, composite publish and the tiny dry execution all passed on Windows x64, net10.0, using SDK 11.0.100-rc.1.26425.128.
  • Before the shared selection change, seven default publishing-selection cases failed because they took the shortcut. After it, all 14 setup/selection tests pass on net10.0 and net472, including the preserved CoreCLR shortcut, dynamic exceptions, non-CLI exclusion and full-framework guard.
  • After removing the explicit Release workaround, both normal R2R integration cases pass locally: actual restore, gather build and final publish each exit 0, with successful dry execution and no --no-dependencies or gather-failure warning.
  • The new test also passed with draft-CI environment flags set; it is discovered by the ordinary integration runner.
  • Earlier SDK 10/11 targeted checks reproduced/cleared NETSDK1067 for generated R2R/Mono projects and preserved final self-contained/composite settings and custom-pack metadata. These targeted checks are not full benchmark executions.
  • After removal of the property theory, the remaining 23 CsProjBuilderTests pass on both net10.0 and net472.
  • git diff --check passes.

CI follow-up

Run 42095481255 exposed a bug in the new test's composite-output assertion on Linux x64 and macOS ARM64. Each integration result had 392 passing tests and only this failure. In both jobs, actual gathering, publishing and benchmark execution succeeded; the failure was at the composite-file check.

Path.ChangeExtension truncated the dotted, extensionless Unix apphost name. The assertion now locates the composite DLL using the generated program name and binaries directory, retaining the nonempty-file check and adding the expected path to its failure message. The repaired integration test passed locally on Windows x64. New Unix CI results remain pending.

Scope and remaining integration limits

The shared rule applies to every CLI builder whose existing publishing decision is true, including R2R, Mono, NativeAOT, Wasm and CoreRun. Local full-process verification is limited to Windows x64/net10.0 R2R under SDK 11 RC1. The other toolchain cases prove selection only, not workload execution, timing or all-platform success.

Existing publishing integration tests now do more dependency/gather work. Existing timeout settings were inspected; no speculative increases or new skips were added. Broader publishing-workload behavior and CI duration remain validation risks for the ordinary PR run.

SDK 12, the original Linux ARM64 R2R-interpreter configuration and actual custom-native-pack execution remain unverified. No new SDK or optional workload was installed.

No manual CI submission, Helix campaign or performance comparison was performed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@timcassell

timcassell commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

I was wondering why our integration tests didn't catch this. I guess it's because we use forced-no-dependencies which skips reference gathering. We should probably have an integration test per toolchain that goes through the full process.

@timcassell timcassell added this to the v0.16.0 milestone Nov 5, 2026
Run the full gather/publish path and verify usable gathered references before checking composite output and execution. Keep this SDK-only case enabled for draft PRs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@timcassell

Copy link
Copy Markdown
Collaborator

Perhaps in BuildPartition.ForcedNoDependenciesForIntegrationTests we can change it back to not include publishing toolchains. DotNetCliBuilder { PublishesOutput: false }

if (RepresentativeBenchmarkCase.GetToolchain().Builder is not DotNetCliBuilder)

That will cover all the publishing toolchains, not just R2R.

Remove the generated-property theory and its test-only access change. Preserve the full R2R regression and locate its composite output by program name, not the extensionless Unix apphost path.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@LoopedBard3
LoopedBard3 marked this pull request as ready for review November 5, 2026 20:46
Keep the no-dependencies shortcut for non-publishing CLI builders. Reuse the existing publish decision internally, cover toolchain selection, and let the R2R regression use the shared path without an explicit Release override.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@timcassell

timcassell commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

@LoopedBard3 I put up a PR against this branch to fail tests when warnings are logged, which also caught a wasm failure. LoopedBard3#2 I will expand it to error logs separately (currently there are too many environmental error logs in CI).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants