Skip to content
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

WIP: SRM and SCI update #25594

Closed
wants to merge 65 commits into from
Closed

WIP: SRM and SCI update #25594

wants to merge 65 commits into from

Conversation

tmat
Copy link
Member

@tmat tmat commented Mar 19, 2018

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

#24677
#23762
#24712
#25185

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

@tmat tmat requested a review from a team as a code owner March 19, 2018 16:59
@tmat tmat requested review from a team as code owners March 20, 2018 00:23
@tmat tmat force-pushed the SRMSCI branch 3 times, most recently from a77e70a to 51ff879 Compare March 21, 2018 18:23
@sharwell
Copy link
Member

Can you include a full technical description of the net46 limitation with this change?

@jaredpar
Copy link
Member

jaredpar commented Mar 22, 2018

Agree it would be nice to have in one of the commits. I can summarize it here though.

Even though Roslyn can only every be deployed where at least net461 or netcoreapp2.0 is available Roslyn cannot depend on netstandard2.0 assemblies. This is due to the setup of Visual Studio which has a minimum target of net46 and hence only deploys netstandard1.3 facades. In order to prevent netstandard2.0 versions of NuGet references to deploy, at build or test, we must limit the desktop target framework used in our projects to net46.

There is nothing functionally wrong with having our tests target net461. At the same time though it would mean our tests were running against the netstandard2.0 version of references, such as System.Collections.Immutable, which is not what we deploy against. Hence not testing what we ship which creates a small but real hole.

@tmat tmat force-pushed the SRMSCI branch 4 times, most recently from acc400c to 7c3a1ea Compare March 23, 2018 06:53
@tmat tmat changed the base branch from dev15.7.x to dev15.7.x-vs-deps March 23, 2018 16:42
jaredpar and others added 27 commits March 30, 2018 10:15
Update PATH to our xcopy CLI
…ompletion doesn't show up when it should have. (#25801)

all these logging is No-op without logging enabled. and they are not enabled in product. only in test.
* foreach to for for C#

* use different method

* no ctor needed

* initial vb support

* added more tests in vb, more code share. more trivia tweak on vb

* more referrence cases

* handle embeded statement case

* some clean up

* addressing some feedbacks. some clean up and etc

* added more unit tests, more refactorings

* assigning better collection name

* more refactoring

* added comment

* more feedback updates

* removed old resource

* use semanticFacts.GenerateUniequeName

* changed to use TypeStypleHelper

* added style test

* fixed test failure

* fix up resx file.

* improve local variable name handling

* updated title.

removed from part.

* share duped code

* missed some places to convert to new dedup method

* more feedbacks

* fix up test failures due ot GenerateUniqueLocalName changes.

* Rename files.
Added both ExpressionBody and BlockBody to the underlying BoundNode f…
Run integration tests on an STA thread
This fixes a signing regression caused be #25751. That changed us to
directly call the SWIX build props / targets vs. getting them implicitly
from the microbuild props / targets. One of the behaviors that the
microbuild props / targets had that wasn't accounted for was the signing
of the VSIX after they are constructed. Hence this lead to signing
errors on insertion.

This PR fixes that by making the following changes:

1. Adds SWIX VSIX to SignToolData.json so they are accounted for during
normal batch signing.
1. Moves the SWIX build to the pre-sign portion of our build.
1. Removes the references from VSMAN -> SWIX. These references are there
only to enforce ordering which is unnecessary. Having them remain risks
that building the VSMAN will cause the SWIX to be re-built which could
possibly invalidate signing.
Fix signing of swix project output
The implementations of ISuggestedAction use threading assertions instead of
automatic transition to the correct thread, placing the burden of correct
thread selection on the caller, TextViewWindow_InProc. This change switches
to the vs-threading approach for maintaining thread affinity, which has two
parts:

1. When entering thread-affinitized code, switch to the correct thread
2. Avoid using ConfigureAwait(false) to lose the required affinity

The entire file relies on the default behavior of ConfigureAwait, which is
the normal coding practice in asynchronous code following vs-threading
patterns.
Make escape analysis over unexpected nodes to return conservative results
Further work to stabilize the integration test interop layer
The getter is 'Enabled' and the setter is 'SetEnabled_OnlyUseExportProviderAttributeCanCall'.
This change ensures that all work added to the work queue after disconnection is
immediately cancelled.
While it doesn't make sense to still be requesting foreground actions after
cancellation is requested, tracking down all callers of
IForegroundNotificationService to ensure the proper checks are in place can be
tedious. This change treats IForegroundNotificationService as the gatekeeper
for scheduling asynchronous operations, and proactively cancels operations if
requested before adding them to the work queue.
Even though Roslyn can only every be deployed where at least net461 or netcoreapp2.0 is available Roslyn cannot depend on netstandard2.0 assemblies. This is due to the setup of Visual Studio which has a minimum target of net46 and hence only deploys netstandard1.3 facades. In order to prevent netstandard2.0 versions of NuGet references to deploy, at build or test, we must limit the desktop target framework used in our projects to net46.

There is nothing functionally wrong with having our tests target net461. At the same time though it would mean our tests were running against the netstandard2.0 version of references, such as System.Collections.Immutable, which is not what we deploy against. Hence not testing what we ship which creates a small but real hole.
@tmat tmat closed this Apr 1, 2018
@tmat tmat deleted the SRMSCI branch June 19, 2018 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants