-
Notifications
You must be signed in to change notification settings - Fork 40
Improve source indexer deduplication to prefer real implementation over *.notsupported.cs files #192
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
Conversation
Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
…er *.notsupported.cs Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
…utput Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
ericstj
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.
@copilot please try plumbing project properties from the binlog as suggested in the feedback.
Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
Implemented project properties plumbing as requested. The BinLogReader now captures project properties from the binlog and stores them in a ProjectProperties dictionary on CompilerInvocation. The scoring logic uses these properties instead of file/command-line conventions. Commit c59f2a1 |
ericstj
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.
@copilot give this a try.
…rder scoring priorities Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
src/SourceBrowser/src/SourceIndexServer.Tests/SourceIndexServer.Tests.csproj
Outdated
Show resolved
Hide resolved
src/SourceBrowser/src/SourceIndexServer/SourceIndexServer.csproj
Outdated
Show resolved
Hide resolved
…BinLogReader Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@joperezr @radical this is ready for review. This tool actually runs inside the repo builds, for example https://dev.azure.com/dnceng/internal/_build/results?buildId=2771328&view=logs&j=b03b9442-9455-54c1-ae98-43ca74c453ac&t=9dac64f1-1058-5d29-d0d5-2337e1b09e97&l=14 I did test this by running the tool against a dotnet/runtime binlog and examining the SLN it generated. It looked good to me, selecting the right project / source files for the index. |
joperezr
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.
LGTM barring those two last comments I left. Thanks @ericstj
Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
src/SourceBrowser/src/BinLogToSln.Tests/InvocationScoringTests.cs
Outdated
Show resolved
Hide resolved
…t score values Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
src/SourceBrowser/src/BinLogToSln.Tests/InvocationScoringTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
joperezr
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.
Great, this looks ready to go! Thanks @ericstj! ![]()
|
Almost, we need to enable tests to run. We should add test step to https://github.com/dotnet/source-indexer/blob/main/.github/workflows/dotnet.yml |
This PR addresses the issue where https://source.dot.net/ shows platform-agnostic source code containing mostly empty classes that throw
PlatformNotSupportedExceptioninstead of the actual platform-specific implementation files.Problem
The current deduplication logic in
BinLogToSln/Program.cswas too simplistic, using only file path-based deduplication. When multiple builds existed for the same assembly (e.g., generic + Linux-specific), it would select whichever was processed first, often resulting in.notsupported.csstub files being chosen over real implementation files.Solution
Implemented improved deduplication logic that:
AssemblyNameinstead of simple file path deduplicationint.MaxValuescore for builds explicitly marked for indexingIsPlatformNotSupportedAssemblybuildsDocumentation and Testing
docs/source-selection-algorithm.mdexplaining the scoring priorities, examples with concrete scores, and configuration optionsBinLogToSln.Testsproject with 12 test methods covering all scoring scenarios, edge cases, and priority ordering to protect against regressionsExample
Before this change, if there were two builds for
System.Net.NameResolution:System.Net.NameResolution.notsupported.cs(score: -1,955)The tool would pick whichever was processed first. Now it consistently selects the Linux build with real implementation.
The changes maintain backward compatibility while significantly improving source quality on https://source.dot.net/.
Fixes #188.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.