React to Roslyn rename of `NullableContextOptions` to `Nullable`. #594

NTaylorMullen · 2019-05-14T04:25:15Z

Verified the fix with VS16.2-preview1 + latest CLI with modified Nullable entry on disk
Updated tests.
Note: This change will not pass the build until we have a new CLI that has an updated Roslyn that understands Nullable.

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

rynowak · 2019-05-14T15:03:52Z

How exactly is this going to work? Are Roslyn changing this for preview 6? This means at some point anyone who wants to use the components/mvc solution will need to get onto 16.2

natemcmaster · 2019-05-14T15:48:38Z

Another wrinkle to consider is source-build and the bootstrapping problem. To build .NET Core of Linux distros, we need a prior version of .NET Core. At the moment, that prior version is the latest stable release (right now Preview 5). We either need a way to stage these kinds of breaking changes between previews, or a way to flow the newer compiler independently of the one bundled with the SDK.

natemcmaster · 2019-05-14T19:04:15Z

@NTaylorMullen try applying this change to your PR: https://github.com/natemcmaster/AspNetCore-Tooling/commit/b416204aa5b5b8773b33b54deb216955ac556a94

NTaylorMullen · 2019-05-14T19:06:41Z

@NTaylorMullen try applying this change to your PR: natemcmaster@b416204

Done, lets see if it builds 😄

src/Razor/test/testapps/Directory.Build.props

natemcmaster · 2019-05-23T00:42:56Z

FYI there is a new version of the compiler coming along soon. The next bot-PR which updates Arcade will have this. cref dotnet/arcade#2856

rynowak · 2019-06-07T02:35:06Z

@NTaylorMullen - we should make this change at our earliest available point.

global.json

NTaylorMullen · 2019-06-20T17:28:43Z

@aspnet/build looks like I need to wait for preview6 SDK to be in Arcade. Any update on that happening soon?

dougbu · 2019-06-21T00:20:05Z

Turns out you should be able to use a newer SDK before then. @JunTaoLuo did you get the needed steps / doc from @markwilkie ?

JunTaoLuo · 2019-06-21T00:21:48Z

Yes, in fact, it has already been applied. I have repro'ed this locally so I'm trying to see if I can figure out what's going on here.

JunTaoLuo · 2019-06-21T06:27:11Z

I've resolved two issues: released preview 6 SDK does not have the compiler option rename NullableContext => Nullable, newer runtimes require a restore with Microsoft.NETCore.App.Host.{RID}.nupkg. The one remaining is that it seems like a different compiler is used for desktop apps? I see the following failures:

    Build_Components_WithDesktopMSBuild_Works [FAIL]
    Build_ProjectWithDependencyThatReferencesMvc_AddsAttribute_WhenBuildingUsingDesktopMsbuild [FAIL]
    Build_SimpleMvc_WithServer_UsingDesktopMSBuild_CanBuildSuccessfully [FAIL]
    Build_SimpleMvc_UsingDesktopMSBuildAndWithoutBuildServer_CanBuildSuccessfully [FAIL]
    Build_SimpleMvc_UsingDesktopMSBuildAndWithoutBuildServer_CanBuildSuccessfully [FAIL]

And they have the signature:

 "C:\Users\vsagent\AppData\Local\Temp\Razor\x1ejfjii.lfs\MvcWithComponents\MvcWithComponents.csproj" (Restore;Build target) (1) ->
      (WarnForExplicitVersions target) -> 
        F:\workspace\_work\1\s\.dotnet\sdk\3.0.100-preview7-012517\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.DefaultItems.targets(192,5): warning NETSDK1071: A PackageReference to 'Microsoft.NETCore.App' specified a Version of `3.0.0-preview7-27819-07`. Specifying the version of this package is not recommended. For more information, see https://aka.ms/sdkimplicitrefs [C:\Users\vsagent\AppData\Local\Temp\Razor\x1ejfjii.lfs\MvcWithComponents\MvcWithComponents.csproj]
      
      
      "C:\Users\vsagent\AppData\Local\Temp\Razor\x1ejfjii.lfs\MvcWithComponents\MvcWithComponents.csproj" (Restore;Build target) (1) ->
      (RazorCompileComponentDeclaration target) -> 
        F:\workspace\_work\1\s\src\Razor\src\Microsoft.NET.Sdk.Razor\build\netstandard2.0\Microsoft.NET.Sdk.Razor.Component.targets(224,10): error MSB4064: The "Nullable" parameter is not supported by the "Csc" task. Verify the parameter exists on the task, and it is a settable public instance property. [C:\Users\vsagent\AppData\Local\Temp\Razor\x1ejfjii.lfs\MvcWithComponents\MvcWithComponents.csproj]
        F:\workspace\_work\1\s\src\Razor\src\Microsoft.NET.Sdk.Razor\build\netstandard2.0\Microsoft.NET.Sdk.Razor.Component.targets(189,5): error MSB4063: The "Csc" task could not be initialized with its input parameters.  [C:\Users\vsagent\AppData\Local\Temp\Razor\x1ejfjii.lfs\MvcWithComponents\MvcWithComponents.csproj]
      
          1 Warning(s)
          2 Error(s)
      
      Time Elapsed 00:00:17.04

@NTaylorMullen can you take a look? I might be able to help out tomorrow if needed, but I'll let you take the first stab at it.

global.json

src/Razor/test/testapps/Directory.Build.props

NTaylorMullen · 2019-06-21T16:41:02Z

released preview 6 SDK does not have the compiler option rename NullableContext => Nullable

Oh damn that's nuts. That means Roslyn inserted a different version into VS for preview6 than exists in the CLI. This is a non-starter for this release then. We don't want to move AspNetCore all the way up to using preview7 bits but that's what this PR would require for both repos. I'll postpone this until preview7 is released and we've moved both our repos to preview7.

@JunTaoLuo Really appreciate your help looking into this, saved us from catastrophic failure 😉.

JunTaoLuo · 2019-06-21T17:49:38Z

Double check the Roslyn version in VS and CLI, I'm surprised that they inserted different versions.

NTaylorMullen · 2019-06-21T17:56:58Z

Double check the Roslyn version in VS and CLI, I'm surprised that they inserted different versions.

It's possible they inserted a preview7 flavor into VS; however, I do know they inserted it because we were exploding VS without their fix 😄

rynowak · 2019-07-15T19:24:18Z

@NTaylorMullen - should this be closed?

NTaylorMullen · 2019-07-16T17:08:07Z

@NTaylorMullen - should this be closed?

Nope, waiting for preview7 to be released and then for this repo to consume the preview7 sdk.

- Verified the fix with VS16.2-preview2 + latest CLI with modified `Nullable` entry on disk - Updated tests. - Note: This change will not pass the build until we have a new CLI that has an updated Roslyn that understands `Nullable`. dotnet/aspnetcore#10218

- This is a temporary workaround until we're able to get on the latest SDK which includes the latest Roslyn bits. Without them we can't properly publish testapps in a standalone fashion without investing in temporary infrastructure.

NTaylorMullen · 2019-07-17T00:27:22Z

src/Razor/test/testapps/Directory.Build.props

@@ -38,4 +38,26 @@

  <Import Project="After.Directory.Build.props" Condition="Exists('After.Directory.Build.props')" />

+  <!--
+    NOTE, BELOW


@pranavkm This entire commit was the reason why the test apps weren't publishing/running properly.

Why aren't these settings coming from /Directory.Build.targets and /eng/Versions.props -- at least indirectly? The Roslyn version there gets updated daily if there are new packages available.

We don't have access to either of those when these apps are tested (they're published to a temp directory).

I could have written logic in the test project to harvest that info but it was a bit more complex than I felt was warranted given all of this is temporary. Once we get a preview 7 SDK I'll delete all of this.

Please do not delete this logic or something that solves the problem without breaking automatic updates. The preview 7 SDK represents a temporary point of time when the Roslyn version aligns.

I'm wondering if this is a problem something in the AspNetCore repo has already solved in its test infrastructure. Probably worth asking around…

dougbu · 2019-07-17T03:21:36Z

src/Razor/test/testapps/Directory.Build.props

+    NOTE, BELOW
+
+    These bits are a temporary workaround until we're able to get on the latest SDK which includes the latest Roslyn bits.
+    Without them we can't properly publish testapps in a standalone fashion without investing in temporary infrastructure.


This shouldn't be a temporary fix. Should always test against the Roslyn version that will ship with the rest of the release.

The SDK isn't relevant in this case either. We have no plans to rev the .NET SDK faster than it is released. While a lot of our build infrastructure is about building and using what's coming in that release, the SDK itself has to stay back.

Of course, this is blocking several repos though. Getting in this hack to unblock that.

Filed an issue to track "doing the right thing": dotnet/aspnetcore#12268

NTaylorMullen requested review from pranavkm, rynowak and natemcmaster May 14, 2019 04:25

NTaylorMullen requested a review from ajaybhargavb as a code owner May 14, 2019 04:25

rynowak reviewed May 14, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved

NTaylorMullen force-pushed the nimullen/10218 branch from 7318395 to c8c8a97 Compare May 14, 2019 19:06

NTaylorMullen requested a review from dougbu as a code owner May 14, 2019 19:06

NTaylorMullen commented May 14, 2019

View reviewed changes

src/Razor/test/testapps/Directory.Build.props Outdated Show resolved Hide resolved

NTaylorMullen force-pushed the nimullen/10218 branch from 94d09b7 to 95f2326 Compare June 7, 2019 16:21

NTaylorMullen requested a review from a team as a code owner June 7, 2019 22:05

NTaylorMullen force-pushed the nimullen/10218 branch from a80b1d8 to 9d1cdd7 Compare June 18, 2019 20:46

NTaylorMullen mentioned this pull request Jun 18, 2019

Rename Razor's NullableContextOptions parameter to Nullable dotnet/aspnetcore#11352

Closed

NTaylorMullen commented Jun 18, 2019

View reviewed changes

global.json Outdated Show resolved Hide resolved

NTaylorMullen force-pushed the nimullen/10218 branch from 9d1cdd7 to 87d57b8 Compare June 19, 2019 17:21

JunTaoLuo force-pushed the nimullen/10218 branch from 87d57b8 to c92ad0a Compare June 21, 2019 05:22

JunTaoLuo reviewed Jun 21, 2019

View reviewed changes

global.json Outdated Show resolved Hide resolved

JunTaoLuo reviewed Jun 21, 2019

View reviewed changes

src/Razor/test/testapps/Directory.Build.props Outdated Show resolved Hide resolved

NTaylorMullen mentioned this pull request Jul 16, 2019

[master] Update dependencies from dotnet/toolset dotnet/installer#3254

Closed

dougbu mentioned this pull request Jul 16, 2019

[master] Update dependencies from 4 repositories dotnet/aspnetcore#12250

Merged

NTaylorMullen force-pushed the nimullen/10218 branch 2 times, most recently from 679bcae to be0b794 Compare July 16, 2019 21:51

Update test projects to use latest Roslyn.

cff58c0

- This is a temporary workaround until we're able to get on the latest SDK which includes the latest Roslyn bits. Without them we can't properly publish testapps in a standalone fashion without investing in temporary infrastructure.

NTaylorMullen force-pushed the nimullen/10218 branch from 23bdc9a to cff58c0 Compare July 17, 2019 00:26

NTaylorMullen commented Jul 17, 2019

View reviewed changes

dougbu reviewed Jul 17, 2019

View reviewed changes

NTaylorMullen merged commit 21fcd9c into master Jul 17, 2019

ghost deleted the nimullen/10218 branch July 17, 2019 05:16

This was referenced Jul 17, 2019

Remove Roslyn integration test Roslyn workaround in tooling dotnet/aspnetcore#12268

Closed

[master] Update dependencies from dotnet/toolset dotnet/installer#3278

Merged

React to Roslyn rename of NullableContextOptions to Nullable. #594

React to Roslyn rename of NullableContextOptions to Nullable. #594

Uh oh!

Conversation

NTaylorMullen commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rynowak commented May 14, 2019

Uh oh!

natemcmaster commented May 14, 2019 via email

Uh oh!

natemcmaster commented May 14, 2019

Uh oh!

NTaylorMullen commented May 14, 2019

Uh oh!

Uh oh!

natemcmaster commented May 23, 2019

Uh oh!

rynowak commented Jun 7, 2019

Uh oh!

Uh oh!

NTaylorMullen commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented Jun 21, 2019

Uh oh!

JunTaoLuo commented Jun 21, 2019

Uh oh!

JunTaoLuo commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NTaylorMullen commented Jun 21, 2019

Uh oh!

JunTaoLuo commented Jun 21, 2019

Uh oh!

NTaylorMullen commented Jun 21, 2019

Uh oh!

rynowak commented Jul 15, 2019

Uh oh!

NTaylorMullen commented Jul 16, 2019

Uh oh!

NTaylorMullen Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

dougbu Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

dougbu Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

React to Roslyn rename of `NullableContextOptions` to `Nullable`. #594

React to Roslyn rename of `NullableContextOptions` to `Nullable`. #594

NTaylorMullen commented May 14, 2019 •

edited

Loading

NTaylorMullen commented Jun 20, 2019 •

edited

Loading

JunTaoLuo commented Jun 21, 2019 •

edited

Loading

NTaylorMullen Jul 17, 2019 •

edited

Loading