-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
RAR as a service - Node startup #5613
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a handful of comments, Thanks for sending out the PR
ref/Microsoft.Build.Tasks.Core/net/Microsoft.Build.Tasks.Core.cs
Outdated
Show resolved
Hide resolved
ref/Microsoft.Build.Tasks.Core/net/Microsoft.Build.Tasks.Core.cs
Outdated
Show resolved
Hide resolved
ref/Microsoft.Build.Tasks.Core/net/Microsoft.Build.Tasks.Core.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs
Outdated
Show resolved
Hide resolved
src/Tasks/ResolveAssemblyReferences/Contract/IResolveAssemblyReferenceTaskHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: started looking at TaskHost, then stopped.
src/Build/BackEnd/Components/Communications/NodeEndpointOutOfProc.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcRar.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a big PR! Do you think it would make sense to break it down into smaller change sets?
I've taken a quick look and left comments.
Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>
…into rarAsService/node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to try to reuse code more rather than duplicating it. It makes it easier to follow, and it means that if we change it in the future, we only need to change it one place rather than multiple.
20e4046
to
268b4d7
Compare
/azp run |
Pull request contains merge conflicts. |
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
/azp run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm happy with this. Thanks!
Co-authored-by: Forgind <Forgind@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a few comments inline. Almost there!
Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a handful of minor comments
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added comments and nits inline.
Typos Co-authored-by: Ladi Prosek <ladi.prosek@gmail.com>
This reverts commit 51a1071, reversing changes made to d58e2b7. This is an overkill solution to dotnet#5752. Since the new functionality isn't working, it's easier to just remove it rather than juggle assembly loading.
This reverts commit 51a1071, reversing changes made to d58e2b7. This is an overkill solution to dotnet#5752. Since the new functionality isn't working, it's easier to just remove it rather than juggle assembly loading.
…ce/node" (dotnet#5758)" This reverts commit d6ef841.
This PR provides MSBuild necessary infrastructure to start up new RAR node and connect to it and perform simple request (just for showcase purpose, will be deleted in next phase).
Still WIP, have to go through the code again and check if there are any possible edge cases. Also I had merged rework of Handshake, so I will have to look into that.
Implements #5555