Skip to content
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

Stop overbuilding SemanticSearch.ReferenceAssemblies #73468

Merged
merged 3 commits into from
May 14, 2024

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer requested a review from a team as a code owner May 14, 2024 06:56
@dotnet-issue-labeler dotnet-issue-labeler bot 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
Member

tmat commented May 14, 2024

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

@tmat
Copy link
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
Member Author

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

@jaredpar jaredpar merged commit 874a204 into dotnet:main May 14, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 14, 2024
@ladipro
Copy link
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
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
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
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