Skip to content

[release/6.0] Try out BuildProjectReferences=false in Components #37146

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

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

github-actions[bot]
Copy link
Contributor

Backport of #37017 to release/6.0

/cc @wtgodbe @BrennanConroy

Customer Impact

Testing

Risk

@github-actions github-actions bot requested a review from a team as a code owner September 30, 2021 21:49
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Sep 30, 2021
@wtgodbe
Copy link
Member

wtgodbe commented Sep 30, 2021

CC @dougbu @BrennanConroy let's wait a couple more days to make sure your change really is making things better in main, then merge this.

Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In preparation, not saying this should be merged today

@dougbu
Copy link
Contributor

dougbu commented Sep 30, 2021

let's wait a couple more days

I agree on the timing. Marked an email to remind me next Wednesday morning 😺

@dougbu
Copy link
Contributor

dougbu commented Oct 6, 2021

@BrennanConroy @captainsafia @Tratcher @TanayParikh please chime in on how well our repo has been building w/ this fix in 'main' for the last week or so. I think @Tratcher mentioned another issue in MVC project but don't have the details at hand.

@TanayParikh
Copy link
Contributor

TanayParikh commented Oct 6, 2021

I think @Tratcher mentioned another issue in MVC project but don't have the details at hand.

This is from the internal issue:

Fresh File-in-use error on main in MVC:
https://dev.azure.com/dnceng/public/_build/results?buildId=1398350&view=logs&j=f43b187f-6c15-531c-a59b-e4fba4e9ed20&t=98a09275-a4dd-593e-c21a-1f8bea3be5b1&l=1747

#37017 was targeted at Components, should it have helped with MVC?

More copy failures:
https://dev.azure.com/dnceng/public/_build/results?buildId=1400266&view=logs&j=6c33d704-163a-5170-38fa-caa7bc99f17c&t=4a2ac175-72f0-54eb-c8b6-eb1e5d86c7d5&l=3230

@dougbu
Copy link
Contributor

dougbu commented Oct 6, 2021

#37017 was targeted at Components, should it have helped with MVC?

Probably not. It appears #37017 may have helped but frequently-used projects like Microsoft.AspNetCore.WebUtilities and Microsoft.Extensions.Features can still build in multiple msbuild instances or threads. For example, the Microsoft.AspNetCore.Mvc.Abstractions.Test ➡️ Microsoft.AspNetCore.Mvc.Core.TestCommon ➡️ Microsoft.AspNetCore.Mvc.Core ➡️ Microsoft.AspNetCore.Http ➡️ Microsoft.AspNetCore.Connections.Abstractions ➡️ Microsoft.AspNetCore.WebUtilities chain is certainly not the only way to reach Microsoft.AspNetCore.WebUtilities. The particular issue might related to multi-targeting in Microsoft.AspNetCore.Connections.Abstractions but multi-targeting doesn't normally hurt us this way.

I suspect we still have a few projects doing unusual things and causing builds of Microsoft.AspNetCore.WebUtilities and Microsoft.Extensions.Features with different global properties. @rainersigwald's tool or a binary log might help go further.

But, let's hear first whether we've hit other problems. Anyone seen anything else❔ I'd appreciate "nos" from @BrennanConroy @captainsafia too.

@TanayParikh
Copy link
Contributor

@TanayParikh please chime in on how well our repo has been building w/ this fix in 'main' for the last week or so.

The past day and half has definitely been smoother than prior rotations where I encountered this issue. I don't believe I've encountered this specific error this time around (mostly the errors I've been seeing have been nuget 503s).

@captainsafia
Copy link
Member

It was pretty smooth for me. I definitely noticed a difference between builds in main vs. release/6.0 so I'm in favor of backporting this.

@dougbu dougbu merged commit 89b3bf0 into release/6.0 Oct 6, 2021
@dougbu dougbu deleted the backport/pr-37017-to-release/6.0 branch October 6, 2021 20:30
@ghost ghost added this to the 6.0.0 milestone Oct 6, 2021
@dougbu
Copy link
Contributor

dougbu commented Oct 6, 2021

Has anyone spent any time up close and personal with a binary log from one of the builds @TanayParikh mentioned❔

@BrennanConroy
Copy link
Member

a binary log from one of the builds @TanayParikh mentioned

Last I checked we don't produce binlogs anymore

chime in on how well our repo has been building w/ this fix in 'main' for the last week or so

I paid attention to builds for the first couple days and it looks like there was a big drop in 'file in use' issues, I know there were still a few, but they seemed to be in slightly different areas of the build now so I think we've made some progress 😃

@dougbu
Copy link
Contributor

dougbu commented Oct 6, 2021

Last I checked we don't produce binlogs anymore

We do though not in every job; right now, it's just Windows_build, Windows_arm_build and Linux_x64_build. Enabling binary logs again in our four aspnetcore-ci test jobs (three platforms plus Helix) may be fine but I've seen problems using those "full build" binary logs. New binary logs for the test jobs also will take up a fair amount more artifact storage and slow those jobs down a bit.

Hmm, the slowdown wouldn't matter if we didn't change the Helix test job because that's normally the slowest in our public builds. Might be worth trying for a week or so in 'main' -- at least long enough to get more information when the next file-in-use or mapped-section-open failure happens❔

@dougbu
Copy link
Contributor

dougbu commented Oct 6, 2021

/fyi our largest _Logs artifact (Windows_Logs) is ~60MB. That's much smaller than the ~209MB Windows_Packages artifact but adding ~70MB per new binary log will add up. Long term, probably want to scale back to just one of the test jobs creating a binary log but I'm fine enabling this in the 3 non-Helix test jobs while we continue peeling this onion.

TanayParikh added a commit that referenced this pull request Oct 6, 2021
@TanayParikh
Copy link
Contributor

but I'm fine enabling this in the 3 non-Helix test jobs while we continue peeling this onion.

#37339

TanayParikh added a commit that referenced this pull request Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants