Repository navigation
Keep reference gathering framework-dependent - #3312
Open
LoopedBard3 wants to merge 4 commits into
Open
LoopedBard3 wants to merge 4 commits into
LoopedBard3 wants to merge 4 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
reviewed
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>
Collaborator
|
Perhaps in 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
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>
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 setsUseAppHost=false. The SDK rejects that combination withNETSDK1067before compilation. Mono has the same conflicting declarations.Add
SelfContained=falseto the existing property group conditioned onBenchmarkDotNetGatherReferences=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):R2RBuilderis identical between them, andCsProjBuilderdiffers only in unrelated comment indentation. This does not establish when the bug was introduced or attribute all consuming CI failures to it.Changes
SelfContained=falseoverride inCsProjBuilder.ForcedNoDependenciesForIntegrationTests, as suggested in the review. The existing non-publishing CLI shortcut, dynamic-build exceptions and integration/.NET Core guards remain intact.R2RToolchainGathersReferencesBeforePublishingin the existing integration suite, now using the ordinary default job with no explicit Release workaround. Both R2R cases take the shared full-process path.The generated-property theory was removed in response to review, along with its helper scaffolding and the test-only
CreateProjectaccessibility 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.
NETSDK1067, and neitherref.txtnorref.propsexisted. Final publish and execution still succeeded, demonstrating why execution-only coverage missed the bug.--no-dependenciesor gather-failure warning.NETSDK1067for generated R2R/Mono projects and preserved final self-contained/composite settings and custom-pack metadata. These targeted checks are not full benchmark executions.CsProjBuilderTestspass on both net10.0 and net472.git diff --checkpasses.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.ChangeExtensiontruncated 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.