Add references to System.IO.Pipes.AccessControl #24457
Merged
Conversation
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
When adding the reference to System.IO.Pipes.AccessControl for the compiler server to use on CoreCLR, I unified the pathway for the desktop and CoreCLR server access control code. This means that System.IO.Pipes.AccessControl needed to be added as a dependent DLL for desktop too, but I forgot to do that. This change adds System.IO.Pipes.AccessControl as a dependent DLL in all the places where the build task is deployed.
heejaechang
reviewed
Jan 25, 2018
| packageVersion, | ||
| isNative:=native IsNot Nothing, | ||
| isFacade:=frameworkAssemblies IsNot Nothing)) | ||
| isFacade:=frameworkAssemblies IsNot Nothing OrElse packageName = "System.IO.Pipes.AccessControl")) |
Contributor
There was a problem hiding this comment.
curious why this is special?
Member
There was a problem hiding this comment.
@heejaechang It's not, the condition should more generally be rewritten. This is a tactical fix.
Member
There was a problem hiding this comment.
One of the happiest days of my future self is deleting this file.
jasonmalinowski
approved these changes
Jan 25, 2018
jaredpar
approved these changes
Jan 25, 2018
Member
Author
|
@MattGertz for escrow approval |
Contributor
|
@agocke Escrow issues need to be submitted to me via VSO bugs. |
Contributor
|
retest windows_release_vs-integration_prtest please |
Contributor
|
the appdomain unloaded exception happened. |
…ild task (true on desktop)
Member
Author
|
@jaredpar After testing on desktop it looks like I needed to adjust the load policy slightly. Can you take a look and sign off again? |
jaredpar
approved these changes
Jan 25, 2018
Member
Author
|
@MattGertz I've added an escrow VSO bug for this PR: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/557536 |
heejaechang
pushed a commit
that referenced
this pull request
Jan 26, 2018
* Add UseCultureAttribute to help with culture-dependent unit tests * Disable NuGet package restore in Visual Studio for Roslyn.sln * Add named argument for literal * Fix behavior for NET46 and NETCOREAPP2_0 * Revert "moved waiter from diagnostics.dll to features.dll where all interface… (#24120)" This reverts commit 823d973. * Add references to System.IO.Pipes.AccessControl (#24457) When adding the reference to System.IO.Pipes.AccessControl for the compiler server to use on CoreCLR, I unified the pathway for the desktop and CoreCLR server access control code. This means that System.IO.Pipes.AccessControl needed to be added as a dependent DLL for desktop too, but I forgot to do that. This change adds System.IO.Pipes.AccessControl as a dependent DLL in all the places where the build task is deployed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Customer scenario
The VS build is currently broken.
When adding the reference to System.IO.Pipes.AccessControl for the
compiler server to use on CoreCLR, I unified the pathway for the desktop
and CoreCLR server access control code. This means that
System.IO.Pipes.AccessControl needed to be added as a dependent DLL for
desktop too, but I forgot to do that. This change adds
System.IO.Pipes.AccessControl as a dependent DLL in all the places where
the build task is deployed.
Bugs this fixes
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/557536
Workarounds, if any
None.
Risk
This change mirrors our behavior for System.IO.Pipes pretty closely. Since any scenario which
needs System.IO.Pipes.AccessControl should also require System.IO.Pipes, it's unlikely that we've
missed a deployment scenario if we cover every scenario where System.IO.Pipes is deployed.
Overall, this change should be validated by RPS.
Performance impact
Without this change, the compiler server does not work on desktop.
Is this a regression from a previous update?
Yes.
Root cause analysis
The bootstrap process we use to test the compiler server in Roslyn is not exactly the same
as the binary layout we use in VS. In addition, we have multiple files which duplicate the
same information about binary layout. @jasonmalinowski has a PR (#22845) to help address the duplication issue. I have filed an issue
(#24456) to move our bootstrap
code closer to the VS layout, which should help catch these issues earlier.
How was the bug found?
RPS Failure