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

Turn on ref assemblies in all projects #21346

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Aug 7, 2017

This dogfoods the end-to-end ref assembly feature in the Roslyn repo.
Some scenarios may be broken (likely LUT and LSL), but the main scenarios have been verified already (for instance, P2P references have been patched, TDD still works).

Relates to #20418

@dotnet/roslyn-infrastructure @jaredpar for review.
FYI @rainersigwald @panopticoncentral @sharwell @Pilchie @ManishJayaswal

@jcouv jcouv added this to the 15.5 milestone Aug 7, 2017
@jcouv jcouv self-assigned this Aug 7, 2017
@jasonmalinowski
Copy link
Member

@jcouv Looks like trying to run the ref assemblies as unit tests ends poorly. ;-)

More seriously, which build did you test this with? Are there any workarounds that we would need the team to know about to enable this?

@@ -22,6 +22,7 @@
<RoslynPortableTargetFrameworks>net46;netcoreapp2.0</RoslynPortableTargetFrameworks>

<Features>strict,IOperation</Features>
<ProduceReferenceAssembly>true</ProduceReferenceAssembly>
Copy link
Member

Choose a reason for hiding this comment

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

📝 It is my understanding that this property alone doesn't really do anything. In order to truly test the end-to-end, we need to add the following as well:

<CompileUsingReferenceAssemblies>true</CompileUsingReferenceAssemblies>

Copy link
Member

Choose a reason for hiding this comment

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

➡️ Looks like this is not necessary:

If you encounter a problem using reference assemblies, you can set the boolean property CompileUsingReferenceAssemblies to false to avoid using ref assemblies even if the projects you reference produce them. This is unset by default and only ever checked against false. It is only there to provide an emergency escape hatch; a customer who hits a bug can set it to false and avoid the new codepaths.

@jcouv
Copy link
Member Author

jcouv commented Aug 7, 2017

@jasonmalinowski The last time I tested this was with private fix from Sam (#20833), which I think went into preview 4 (I'd have to double-check).
I also did a quick validation this morning with Visual Studio 2017 version 15.3 preview 7.0.

@sharwell
Copy link
Member

sharwell commented Aug 7, 2017

I think the issue here is we use a wildcard path to discover the xunit test assemblies. If you compare say this build output (new) with this build output (old), you can see that adding this flag resulted in many additional test assemblies getting discovered. We need to disable attempts to run the xunit test runner on reference assemblies.

@sharwell
Copy link
Member

sharwell commented Aug 7, 2017

@jcouv You need to add a new exclusion like this one.

@jaredpar
Copy link
Member

jaredpar commented Aug 7, 2017

@jcouv @sharwell is right we will likely need additional exclusions now. Grab me when you have 5 and I can help add those.

@@ -347,6 +347,10 @@ function Test-XUnit() {
# Exclude out the multi-targetted netcore app projects
$dlls = $dlls | ?{ -not ($_.FullName -match ".*netcoreapp.*") }

# Exclude out the ref assemblies
$dlls = $dlls | ?{ -not ($_.FullName -match ".*\ref\.*") }
Copy link
Member

Choose a reason for hiding this comment

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

📝 If this is regular expressions, these need to use \\.

@jcouv
Copy link
Member Author

jcouv commented Aug 8, 2017

windows_release_unit32_prtest failed in Microsoft.CodeAnalysis.Editor.UnitTests.LinkedFiles.LinkedFileDiffMergingEditorTests.TestCodeActionPreviewAndApply (details)

FYI @dotnet/roslyn-infrastructure

@jcouv
Copy link
Member Author

jcouv commented Aug 8, 2017

windows_debug_unit32_prtest failed with error below (details)

error CS0016: Could not write to output file 'D:\j\workspace\windows_debug---09a07fba\Binaries\Obj\TestUtilities\Debug\Roslyn.Test.Utilities.xml' -- 'The process cannot access the file 'D:\j\workspace\windows_debug---09a07fba\Binaries\Obj\TestUtilities\Debug\Roslyn.Test.Utilities.xml' because it is being used by another process.' [D:\j\workspace\windows_debug---09a07fba\src\Test\Utilities\Portable\TestUtilities.csproj]

@jcouv
Copy link
Member Author

jcouv commented Aug 8, 2017

test windows_debug_unit32_prtest please

@jcouv
Copy link
Member Author

jcouv commented Aug 8, 2017

test windows_release_unit32_prtest please

@jcouv
Copy link
Member Author

jcouv commented Aug 8, 2017

Updated the regexes with proper escape sequences. Thanks @sharwell
I'l go ahead and merge.

@jcouv jcouv merged commit 414d7d6 into dotnet:master Aug 8, 2017
@jcouv jcouv deleted the enable-refout branch August 8, 2017 17:07
333fred added a commit to 333fred/roslyn that referenced this pull request Aug 10, 2017
…-literal-text

* dotnet/features/ioperation:
  Fix NamedArgumentInParameterOrderWithDefaultValue test for new IOperation output.
  EnC and EE cleanup (dotnet#21226)
  Fix crash when encountering a parenthesized expression when converting an if to a switch.
  this makes OOM to crash OOP process. this won't crash VS, instead it will show info bar and notify users to close VS and re-open.
  Turn on ref assemblies in all projects (dotnet#21346)
  Re-baseline some emit tests
  Move MakeFrames logic into Analysis
  Use langver=latest for scripting (dotnet#21331)
  Enable skipped tests and fix them (dotnet#21335)
  Replace project reference with linked file
  removed left out from deleted esent code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants