Skip to content

Try out BuildProjectReferences=false in Components #37017

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
Sep 29, 2021
Merged

Conversation

BrennanConroy
Copy link
Member

Same change as #34137 which was made to try and reduce build flakiness.
https://github.com/dotnet/aspnetcore-internal/issues/3915#issuecomment-928011626 for more info

@BrennanConroy BrennanConroy requested a review from a team September 27, 2021 17:00
@BrennanConroy BrennanConroy requested a review from a team as a code owner September 27, 2021 17:00
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Sep 27, 2021
@BrennanConroy
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dougbu
Copy link
Contributor

dougbu commented Sep 27, 2021

GitHub and AzDO are very confused: GitHub tried to cancel previous build for this PR but a few jobs are still running☹️

🤞 AzDO and GitHub eventually catch up (and that this change works 😃)

@BrennanConroy
Copy link
Member Author

GitHub and AzDO are very confused

That's what I get for doing manual builds before opening a PR :D

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.

WFM though I'm surprised this doesn't cause new and different ordering issues e.g. Wasm.Prerendered.Server.csproj dependencies not being ready when Microsoft.AspNetCore.Components.E2ETests.csproj asks Wasm.Prerendered.Server.csproj to build.

@dougbu
Copy link
Contributor

dougbu commented Sep 27, 2021

That's what I get for doing manual builds before opening a PR :D

Recommend draft PRs unless you need to test things w/ internal builds.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Sep 27, 2021

WFM though I'm surprised this doesn't cause new and different ordering issues e.g. Wasm.Prerendered.Server.csproj dependencies not being ready when Microsoft.AspNetCore.Components.E2ETests.csproj asks Wasm.Prerendered.Server.csproj to build.

Yes, I wondered too. Does this change mean we're just hoping the build ordering/parallelism happens to work out OK, or is there anything that ensures it? I'd be concerned if we're merging this on the basis that it happens to pass in CI by chance.

Also when building the E2ETests project locally, does it mean it no longer rebuilds any dependencies that have been changed? Because if so that would harm productivity for us and contributors. Update: not really an issue because this change doesn't apply to debug builds.

Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Isn't this problematic? If there's a reference that is unique to one of these projects, it'll never get built?

@dougbu
Copy link
Contributor

dougbu commented Sep 27, 2021

@pranavkm that part I get 😄 The item group @BrennanConroy is changing is only used on the CI or when building the Release configuration. See

@BrennanConroy
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@BrennanConroy
Copy link
Member Author

Running again for sanity, planning on merging if it passes again.

@SteveSandersonMS
Copy link
Member

Running again for sanity, planning on merging if it passes again.

Asking again, with this change, is there anything that causes the build ordering/parallelism to be valid, or does it just happen to work by chance?

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Sep 28, 2021

is there anything that causes the build ordering/parallelism to be valid, or does it just happen to work by chance?

Components being near the end of the stack is probably why this just works. I understand your ordering concerns but I'd rather try this out to see if the 'file in use' issues stop happening and revert/file an issue if we see ordering issues. Right now we have little to no information about the build problems we've been seeing and this PR will hopefully give us some info.

Plus, Components already has this change for another of the project references so build ordering isn't a new problem here, it just might be more likely now.

@dougbu
Copy link
Contributor

dougbu commented Sep 29, 2021

is there anything that causes the build ordering/parallelism to be valid, or does it just happen to work by chance?

The changed project also has a lot of dependencies, likely covering (almost❔) everything the few dependencies passed $(BuildProjectReferences) need. Those dependencies also appear at the end of the item group and will be processed last.

@SteveSandersonMS
Copy link
Member

OK, sounds reasonable! We'll have to see how this turns out in practice.

@BrennanConroy BrennanConroy merged commit fc0a408 into main Sep 29, 2021
@BrennanConroy BrennanConroy deleted the brecon/bpr branch September 29, 2021 14:55
@ghost ghost added this to the 7.0-preview1 milestone Sep 29, 2021
@wtgodbe
Copy link
Member

wtgodbe commented Sep 30, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1292816098

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