Skip to content

Stop overbuilding SemanticSearch.ReferenceAssemblies#73468

Merged
jaredpar merged 3 commits intodotnet:mainfrom
ViktorHofer:patch-3
May 14, 2024
Merged

Stop overbuilding SemanticSearch.ReferenceAssemblies#73468
jaredpar merged 3 commits intodotnet:mainfrom
ViktorHofer:patch-3

Conversation

@ViktorHofer
Copy link
Copy Markdown
Member

@ViktorHofer ViktorHofer requested a review from a team as a code owner May 14, 2024 06:56
@ghost ghost added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels May 14, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 14, 2024
@tmat
Copy link
Copy Markdown
Member

tmat commented May 14, 2024

@ViktorHofer Thanks for the fix! We couldn't figure out what the issue is.

@tmat
Copy link
Copy Markdown
Member

tmat commented May 14, 2024

@ladipro How come msbuild lock detection was not able to find out that the file is locked by msbuild itself? Is it missing anything?

@ViktorHofer
Copy link
Copy Markdown
Member Author

Feel free to merge. I don't have permissions.

@jaredpar jaredpar merged commit 874a204 into dotnet:main May 14, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 14, 2024
@ladipro
Copy link
Copy Markdown
Member

ladipro commented May 14, 2024

@ladipro How come msbuild lock detection was not able to find out that the file is locked by msbuild itself? Is it missing anything?

Competing writes to obj files, such what's in the description of the referenced issue, come from CSC so that's outside of MSBuild; it would have to be implemented in the compiler. For files in bin, so typically copying with the Copy task, my guess is that the operation is too fast that by the time the process detection logic runs, the file is no longer locked.

As mentioned in the issue, we're working on a double-writes analyzer. Interestingly though, binlog viewer already has double-writes analysis and it didn't catch this. It deserves a closer look because we were going to model MSBuild analysis after the viewer.

@ViktorHofer ViktorHofer deleted the patch-3 branch May 14, 2024 15:53
@ladipro
Copy link
Copy Markdown
Member

ladipro commented May 14, 2024

Oh yes, currently the output path itself is used as a key which explains why it doesn't detect double writes in this case. I would naively expect the data to live in a list (i.e. no need to de-dup with a HashSet). Cc @KirillOsenkov for insights.

https://github.com/KirillOsenkov/MSBuildStructuredLog/blob/b56ebfb2caea11a3ffad279aa25ddd0f67a115da/src/StructuredLogger/Analyzers/DoubleWritesAnalyzer.cs#L72,L96

@tmat
Copy link
Copy Markdown
Member

tmat commented May 14, 2024

@ladipro Makes sense in this case - I think we already have a work item to expose the process locking detection logic to tasks, so that csc could also report the process name using msbuild APIs.

@KirillOsenkov
Copy link
Copy Markdown
Member

I filed a viewer bug KirillOsenkov/MSBuildStructuredLog#779 to deal with this case.

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

Labels

Area-Infrastructure Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Building roslyn in the VMR fails non-deterministically

6 participants