Fix missing project instance in project cache requests#6568
Merged
Conversation
cdmihai
changed the title
rainersigwald
approved these changes
Jun 18, 2021
Comment on lines
+1356
to
+1357
| // TODO: Reenable when this gets into the main branch. | ||
| //[InlineData(true, true)] |
Member
There was a problem hiding this comment.
When what gets into which main branch?
Contributor
Author
There was a problem hiding this comment.
Literal meaning. When this comment gets into the main branch, the test should be reenabled :)
Member
There was a problem hiding this comment.
ah, so there's a fix in our main that's not in our 16.11 that means we should change this on merge?
Contributor
Author
There was a problem hiding this comment.
Correct, the fix is in #6400 which we deemed to risky for 16.11
Forgind
approved these changes
Jun 18, 2021
|
|
||
| return await GetCacheResultAsync(cacheRequest.Submission.BuildRequestData); | ||
| return await GetCacheResultAsync( | ||
| new BuildRequestData( |
Contributor
There was a problem hiding this comment.
Do you think it would make sense to check whether the ProjectInstance is set?
Contributor
Author
There was a problem hiding this comment.
Good catch, added it.
Forgind
added
the
merge-when-branch-open
rokonec
pushed a commit
that referenced
this pull request
Aug 5, 2021
* Fix missing project instance in project cache requests (#6568) Context Non static graph builds using the project cache didn't set the ProjectInstance on the cache request, leading to crashes in the cache. Changes Made Recreate the BuildRequestData for the cache request after the cache service evaluates projects. I was initially using the original BuildSubmission.BuildRequestData which does not contain the project instance. * Don't launch debugger window for all tests * Convert static InitializePlugin into non-static BeginBuildAsync To allow asserting service state transition * Assert state transitions in ProjectCacheService * Only initialize once for the VS workaround * Bravely set DoNotLaunchDebugger only once for all tests * Simplify branching * [vs16.11] Update dependencies from dotnet/arcade (#6625) * Update dependencies from https://github.com/dotnet/arcade build 20210628.3 Microsoft.DotNet.Arcade.Sdk From Version 5.0.0-beta.21315.2 -> To Version 5.0.0-beta.21328.3 * Don't schedule proxy builds to inproc node if their configs previously built on oop nodes (#6635) Fixes a bug in proxy build scheduling introduced by #6386. If a the BuildRequestConfiguration associated with a proxy request has been built before on an out of proc (oop) node then the scheduler will fail with either one of: - affinity mismatch error. This happens when the proxy build is assigned to the inproc (inp) node but its configuration is already assigned to an oop node `AND` serving other existing requests, either blocked or running. - unscheduled requests remain even if there's free oop nodes that can serve them. This happens (as far as I can tell) when the proxy's configuration is already assigned to an oop node (because a previously built non proxy request was assigned there) `AND` there's no other existing requests for that configuration The fix in this PR is to not assign a proxy build to the inproc node if its configuration was previously assigned to another node. * Localized file check-in by OneLocBuild Task (#6644) * 16.11 Final Branding (#6656) Co-authored-by: Rainer Sigwald <raines@microsoft.com> Co-authored-by: Mihai Codoban <micodoba@microsoft.com> Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org> Co-authored-by: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Co-authored-by: Rainer Sigwald <raines@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Non static graph builds using the project cache didn't set the ProjectInstance on the cache request, leading to crashes in the cache.
Changes Made
Recreate the BuildRequestData for the cache request after the cache service evaluates projects. I was initially using the original BuildSubmission.BuildRequestData which does not contain the project instance.
Testing
Unit tests
Notes
This does not affect non project cache code paths so it shouldn't be a risk for 16.11.