Skip to content

Remove BuildX64 target #41040

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
Apr 6, 2022
Merged

Remove BuildX64 target #41040

merged 1 commit into from
Apr 6, 2022

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Apr 5, 2022

I'm opening this draft PR to see how removing the BuildX64 target in InProcessWebSite added in #39998 causes dotnet publish --no-build to fail when running tests on the CI. During dev ops I saw a lot of file copy failures and the deleted call to <MSBuild Projects="$(MSBuildProjectFullPath)" Properties="RuntimeIdentifier=win-x64;ReferenceTestTasks=false" Targets="Build" /> seems to be to blame.

image

#32219 We've had a bunch of things cause issues with file locks on the CI, but this seems especially frequent during my build ops rotation.

@ghost ghost added the area-runtime label Apr 5, 2022
@halter73 halter73 requested a review from dougbu April 5, 2022 00:54
@halter73
Copy link
Member Author

halter73 commented Apr 5, 2022

I'm not sure the file locking is the fault of the Build64 target. It's probably more likely that it's the unlucky victim that just happens to be common with all the failures.

I'm suspicious that an IIS functional test is deploying the InProcessWebSite while another functional test is still building. What would be the best way to confirm that? I'm struggling to find what's running in parallel with failing ResolveProjectReferences calls using timeline view in MSBuild Structured Log Viewer.

Edit: I was able to find the failed tasks in the tracing view. There doesn't look to be any other parallel tasks.

@dougbu
Copy link
Member

dougbu commented Apr 5, 2022

The runs linked in the description are all about file copy problems in general i.e., #32219. What pointed you toward these two projects instead of the projects mentioned in those errors❔

see how removing the BuildX64 target in InProcessWebSite …

Without these targets, we're unable to test the standalone deployment option at all. I didn't look at the CI failures in this PR but suspect they failed for that reason.

There doesn't look to be any other parallel tasks.

Odd because our build tends to do a bunch of work in every msbuild instance for the duration of the build.

@halter73
Copy link
Member Author

halter73 commented Apr 5, 2022

The runs linked in the description are all about file copy problems in general i.e., #32219

If you look at the binlogs of all three failures that I linked to from just yesterday, all of the failures happened inside the BuildX64 target of InProcessWebSite.

I know we've had a bunch of file locking issues on the CI from my own PRs and previous build ops rotations, but I find it very suspicious that all the file locking issues I looked at yesterday happened to be while running InProcessWebSite's BuildX64 target.

Without these targets, we're unable to test the standalone deployment option at all. I didn't look at the CI failures in this PR but suspect they failed for that reason.

Only the Microsoft.AspNetCore.ConcurrencyLimiter.Tests Helix tests failed on macOS which cannot be related to the InProcessWebSite changes. aspnetcore-ci (Build Test: Windows Server x64) and the Windows Helix tests ran fine which is what I concerned about.

There doesn't look to be any other parallel tasks.

Odd because our build tends to do a bunch of work in every msbuild instance for the duration of the build.

I meant to write the three linked failures don't have any other parallel tasks in common but forget to finish my sentence. Two of the failures had LinkabilityChecker's ILLink target running in parallel, but not the third.

@halter73 halter73 marked this pull request as ready for review April 5, 2022 19:34
BeforeTargets="Build"
Condition=" '$(RuntimeIdentifier)' != 'win-x64' AND '$(TargetArchitecture)' != 'arm' ">
<MSBuild Projects="$(MSBuildProjectFullPath)"
Properties="RuntimeIdentifier=win-x64;ReferenceTestTasks=false"
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding BuildProjectReferences=false to these Properties instead of removing the whole target. Same in the other project

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this target necessary at all given that all the tests are passing?

Copy link
Member

Choose a reason for hiding this comment

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

You're right, these two <Target/>s are now redundant and cause problems.

Took a bit to find the change that made these <Target/>s unnecessary. They were mandatory back when I did #39998 because the Publish target was only run in the Helix build step and that step uses -noBuild. Publish fails if the project hasn't yet been built (in this case, for the $(RuntimeIdentifier) of interest).

This changed when I did #40379 because the CopyAssets target in FunctionalTest.props now runs in the prior build step, without -noBuild.

BeforeTargets="Build"
Condition=" '$(RuntimeIdentifier)' != 'win-x64' AND '$(TargetArchitecture)' != 'arm' ">
<MSBuild Projects="$(MSBuildProjectFullPath)"
Properties="RuntimeIdentifier=win-x64;ReferenceTestTasks=false"
Copy link
Member

Choose a reason for hiding this comment

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

You're right, these two <Target/>s are now redundant and cause problems.

Took a bit to find the change that made these <Target/>s unnecessary. They were mandatory back when I did #39998 because the Publish target was only run in the Helix build step and that step uses -noBuild. Publish fails if the project hasn't yet been built (in this case, for the $(RuntimeIdentifier) of interest).

This changed when I did #40379 because the CopyAssets target in FunctionalTest.props now runs in the prior build step, without -noBuild.

@halter73 halter73 merged commit 602b3f7 into main Apr 6, 2022
@halter73 halter73 deleted the halter73/remove-buildx64 branch April 6, 2022 22:16
@ghost ghost added this to the 7.0-preview4 milestone Apr 6, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants