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

Upgrade to more recent .NET SDK #21017

Merged
merged 4 commits into from
May 6, 2020
Merged

Upgrade to more recent .NET SDK #21017

merged 4 commits into from
May 6, 2020

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Apr 20, 2020

No description provided.

@dougbu dougbu added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 20, 2020
@dougbu dougbu requested a review from a team April 20, 2020 16:34
@dougbu
Copy link
Member Author

dougbu commented Apr 20, 2020

/fyi @ViktorHofer @lukas-lansky @riarenas

@dougbu
Copy link
Member Author

dougbu commented Apr 20, 2020

I believe the new SDK brings along a number of breaking changes. New errors and warnings include

F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-dev-certs\Release\netcoreapp5.0\publish\dotnet-dev-certs.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-dev-certs.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-dev-certs\src\dotnet-dev-certs.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-dev-certs\Release\netcoreapp5.0\publish\dotnet-dev-certs.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-dev-certs.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-dev-certs\src\dotnet-dev-certs.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-sql-cache\Release\netcoreapp5.0\publish\dotnet-sql-cache.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-sql-cache.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-sql-cache\src\dotnet-sql-cache.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-user-secrets\Release\netcoreapp5.0\publish\dotnet-user-secrets.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-user-secrets.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-user-secrets\src\dotnet-user-secrets.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-user-secrets\Release\netcoreapp5.0\publish\dotnet-user-secrets.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-user-secrets.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-user-secrets\src\dotnet-user-secrets.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-sql-cache\Release\netcoreapp5.0\publish\dotnet-sql-cache.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-sql-cache.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-sql-cache\src\dotnet-sql-cache.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-watch\Release\netcoreapp5.0\publish\dotnet-watch.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-watch.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-watch\src\dotnet-watch.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\dotnet-watch\Release\netcoreapp5.0\publish\dotnet-watch.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-watch.deps.json' [F:\workspace\_work\1\s\src\Tools\dotnet-watch\src\dotnet-watch.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\Microsoft.dotnet-openapi\Release\netcoreapp5.0\publish\dotnet-openapi.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-openapi.deps.json' [F:\workspace\_work\1\s\src\Tools\Microsoft.dotnet-openapi\src\Microsoft.dotnet-openapi.csproj]
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5118: File 'F:\workspace\_work\1\s\artifacts\bin\Microsoft.dotnet-openapi\Release\netcoreapp5.0\publish\dotnet-openapi.deps.json' is not added because the package already contains file 'tools\netcoreapp5.0\any\dotnet-openapi.deps.json' [F:\workspace\_work\1\s\src\Tools\Microsoft.dotnet-openapi\src\Microsoft.dotnet-openapi.csproj]
...
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5129: - At least one .targets file was found in 'buildTransitive/netcoreapp5.0/', but 'buildTransitive/net5.0/Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.targets' was not. [F:\workspace\_work\1\s\src\Mvc\Mvc.Razor.RuntimeCompilation\src\Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.csproj]
...
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5129: - At least one .targets file was found in 'buildTransitive/netcoreapp5.0/', but 'buildTransitive/net5.0/Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.targets' was not. [F:\workspace\_work\1\s\src\Mvc\Mvc.Razor.RuntimeCompilation\src\Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.csproj]
...
F:\workspace\_work\1\s\.dotnet\sdk\5.0.100-preview.4.20202.8\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): warning NU5129: - At least one .targets file was found in 'build/netcoreapp5.0/', but 'build/net5.0/Microsoft.AspNetCore.Mvc.Testing.targets' was not. [F:\workspace\_work\1\s\src\Mvc\Mvc.Testing\src\Microsoft.AspNetCore.Mvc.Testing.csproj]
...
Unhandled exception. System.IO.DirectoryNotFoundException: Could not find a part of the path 'F:\workspace\_work\1\s\artifacts\obj\InProcessWebSite\Release\netcoreapp5.0\win-x64\x64\aspnetcorev2_inprocess.dll'.
     at System.IO.FileSystem.CopyFile(String sourceFullPath, String destFullPath, Boolean overwrite)
     at System.IO.File.Copy(String sourceFileName, String destFileName, Boolean overwrite)
     at TestTasks.InjectRequestHandler.Main(String[] args) in F:\workspace\_work\1\s\src\Servers\IIS\IIS\test\testassets\TestTasks\InjectRequestHandler.cs:line 61
F:\workspace\_work\1\s\src\Servers\IIS\build\testsite.props(73,5): error MSB3073: The command "dotnet F:\workspace\_work\1\s\src\Servers\IIS\build\..\IIS\test\testassets\TestTasks\bin\Release\netcoreapp5.0\TestTasks.dll "F:\workspace\_work\1\s\artifacts\obj\InProcessWebSite\Release\netcoreapp5.0\win-x64\InProcessWebSite.deps.json" win-x64 " exited with code -532462766. [F:\workspace\_work\1\s\src\Servers\IIS\IIS\test\testassets\InProcessWebSite\InProcessWebSite.csproj]
    24 Warning(s)
    1 Error(s)

The final error implies native assets aren't being copied where they're expected.

So, @ViktorHofer @dsplaisted @rainersigwald @rrelyea what's changed and how do we need to react in this repo?

@rrelyea
Copy link

rrelyea commented Apr 20, 2020

@dougbu
Copy link
Member Author

dougbu commented Apr 20, 2020

@rrelyea

  • We're not using the net5.0 TFM at all in this repo (yet). The projects where NU5118 warnings occur all use <PackAsTool>true</PackAsTool>.
  • There's no misalignment between the package IDs and the "missing" *.targets files. It's much more like NuGet or the .NET SDK no longer support the netcoreapp5.0 TFM.

FYI we don't do much more than the following in the projects showing NU5129 warnings.

  <ItemGroup>
    <None Include="targets\$(PackageId).targets" Pack="true" PackagePath="build\$(DefaultNetCoreTargetFramework)\$(Packa
geId).targets" />
    <None Include="targets\$(PackageId).targets" Pack="true" PackagePath="buildTransitive\$(DefaultNetCoreTargetFramewor
k)\$(PackageId).targets" />
  </ItemGroup>

Are you saying the fix is to change our $(DefaultNetCoreTargetFramework) property to net5.0 and workaround the fact Microsoft.NetCore.App still uses the netcoreapp5.0 TFM? (That sounds a bit scary.)

@rrelyea
Copy link

rrelyea commented Apr 20, 2020

what build of sdk were you using before? what version are you upgrading to?

where you see us treating netcoreapp5.0 as net5.0, and you are running into problems...
the right long term thing to do is move to net5.0.

until then, you may be able to live with ignoring the warning (via nowarn)?
looking at previous packages that were built before this happened may help us determine what files the output package is created with.

@zkat

@dougbu
Copy link
Member Author

dougbu commented Apr 20, 2020

@rrelyea this PR makes the following change

-    "version": "5.0.100-preview.2.20120.3"
+    "version": "5.0.100-preview.4.20202.8"

@dougbu
Copy link
Member Author

dougbu commented Apr 20, 2020

until then, you may be able to live with ignoring the warning (via nowarn)?

That won't solve the error shown above in #21017 (comment).

@rrelyea
Copy link

rrelyea commented Apr 20, 2020

by error, you mean the exception?

we generally tried to leave netcoreapp5.0 working in parallel with net5.0.
seems like these projects may be problematic.
that exception looks like it is looking for a file (with netcoreapp5.0 in the path).
most likely, that file is next to it (with net5.0 in the path).

so you need to figure out if you can move forward, or if you need to stay back.
if you have to stay back, you might need to make your test understand the hybrid state you are in.

when does Microsoft.NetCore.App get fixed and by who?

@dougbu
Copy link
Member Author

dougbu commented Apr 20, 2020

@rrelyea why would $(IntermediateOutputPath) be artifacts\obj\InProcessWebSite\Release\net5.0\win-x64\x64\aspnetcorev2_inprocess.dll when the $(OutputPath) is bin\Release\netcoreapp5.0\InProcessWebSite.dll? (I'm adding more logging to the failing InjectRequestHandler class.)

when does Microsoft.NetCore.App get fixed and by who?

@ericstj @ViktorHofer I don't see any mention of net5.0 in dotnet/runtime yet. When are you planning to make the move?

If that's not coming soon, we can't change our default TFM until we get VS16.6 on the agents or I resolve the problems seen in #20748. The short-term workaround would be to update InjectRequestHandler and ignore the new NuGet warnings.

@ViktorHofer
Copy link
Member

We will switch to net5.0 on May 4th: dotnet/runtime#35202

@dougbu
Copy link
Member Author

dougbu commented Apr 22, 2020

My added logging is dumping into /dev/null at the moment -- forgot stdout is mostly lost in our test frameworks. Working on local reproduction to figure out where the in-proc handler actually lands.

@dougbu
Copy link
Member Author

dougbu commented Apr 27, 2020

There are a couple of things going on here:

  1. With the new SDK, restore operations frequently hit the following error:

    C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(128,5): error : Cannot create a file when that file already exists. [C:\dd\dnx\aspnetcore\src\Servers\IIS\IISIntegration\test\Tests\Microsoft.AspNetCore.Server.IISIntegration.Tests.csproj]
    

    Unfortunately there is no information in the binary log about which file already exists. Beyond seeing multiple msbuild processes restoring the same project e.g. Microsoft.AspNetCore.Testing, the symptoms aren't clear.

    @rainersigwald and @rrelyea are the build / restore parallelism issues an expected regression? Are there workarounds or ways to debug the issue?

  2. InjectRequestHandler in TestTasks.dll is given $(PublishDepsFilePath) as one of its arguments and, for the failing command, that property value has changed from

    bin\Release\netcoreapp5.0\publish\\InProcessWebSite-Standalone-x64\InProcessWebSite.deps.json
    

    in 'master' to the following in this branch

    artifacts\obj\InProcessWebSite\Release\netcoreapp5.0\win-x64\InProcessWebSite.deps.json
    

    The value change doesn't matter when the command is also given an empty $(RuntimeIdentifier). But, when the second argument is non-empty, InjectRequestHandler attempts to copy aspnetcorev2_inprocess.dll within the (supposed) publish/ directory. Since the command is actually given a *.deps.json path within obj/ and aspnetcorev2_inprocess.dll is only copied into bin/ and publish/ directories, the copy fails.

    The root cause of the problem is a breaking change in Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets, from the following in 5.0.100-preview.2.20120.3

      <PublishDepsFilePath Condition=" '$(PublishDepsFilePath)' == '' And '$(PublishSingleFile)' != 'true'">$(PublishDir)$(ProjectDepsFileName)</PublishDepsFilePath>
      <PublishDepsFilePath Condition=" '$(PublishDepsFilePath)' == '' And '$(PublishSingleFile)' == 'true'">$(IntermediateOutputPath)$(ProjectDepsFileName)</PublishDepsFilePath>
    

    to the following in 5.0.100-preview.4.20202.8

    <PublishDepsFilePath Condition=" '$(PublishDepsFilePath)' == ''">$(IntermediateOutputPath)$(ProjectDepsFileName)</PublishDepsFilePath>
    

    (The GeneratePublishDependencyFile target has also been removed or renamed.)

    @swaroop-sridhar and @dsplaisted was the breaking change in dotnet/sdk@1f2a4a1#diff-7dc833d02f198e5a9f0ab88e229db18aL996-R980 intended? We might be able to work around the issue but I'd rather not if it'll be fixed soon. (Since Adjust ordering of Single-file publish targets sdk#10740 was about single-file publish operations, I'm surprised the behaviour of regular publish ops changed at all.)

@swaroop-sridhar
Copy link

Was the breaking change in dotnet/sdk@1f2a4a1#diff-7dc833d02f198e5a9f0ab88e229db18aL996-R980 intended? We might be able to work around the issue but I'd rather not if it'll be fixed soon. (Since dotnet/sdk#10740 was about single-file publish operations, I'm surprised the behaviour of regular publish ops changed at all.)

@dougbu The change to generate the deps.json file in the obj directory for all builds was intentional -- because it simplifies the ResolvedFileToPublish computation and ordering of certain publish targets.

However, the definition of PublishDepsFilePath could be left unchanged (for non-single-file cases).
For example, I can change the definition such that PublishDepsFilePath is:

  • $(PublishDir)$(ProjectDepsFileName) for normal publish
  • Empty for single-file publish.

You could use $(PublishDir)$(AssemblyName).deps.json in the test code to circumvent the problem in the meantime.

@dougbu
Copy link
Member Author

dougbu commented Apr 27, 2020

You could use $(PublishDir)$(AssemblyName).deps.json in the test code to circumvent the problem in the meantime.

We also need to edit the .deps file. adding information about a native assembly. That's why our command executes after GeneratePublishDependencyFile. Does $(PublishDir)$(AssemblyName).deps.json exist once GeneratePublishDependencyFile is done?

Since simplifying InjectRequestHandler to avoid the copy didn't work, I'm instead going to look at passing the $(PublishDir) directory on the command line.

@dougbu
Copy link
Member Author

dougbu commented Apr 27, 2020

But,

I can change the definition such that PublishDepsFilePath is

sounds better to me.

@dougbu
Copy link
Member Author

dougbu commented Apr 27, 2020

… because that restores the previous values and customers may also be depending on them.

@ViktorHofer
Copy link
Member

@dougbu please update to 5.0.100-preview.5.20227.9 which contains a fix for a runtime regression that we had in for ~20 days: dotnet/runtime@a5414a2

@dougbu
Copy link
Member Author

dougbu commented May 4, 2020

🆙📅 to grab latest from 'master' branch and fix the @(AssemblyAttribute) use in the Blazor and project template tests. Unfortunately, it's doubtful this will fix the timeouts in the macOS quarantined test runs.

@BrennanConroy
Copy link
Member

Same or similar issue repros locally. Does a new SDK bring a new hostfxr? I am attached to the hung process right now and it is stuck inside of native code in libcoreclr.dylib, and I noticed that there are some recent changes to that section of code in the Runtime. So I'd like to know if the host can be updated by a SDK update since this PR isn't bringing in a new Runtime.

@BrennanConroy
Copy link
Member

Oh, the shared runtime did update, I didn't notice that. So, this might be a regression in hostfxr.

@BrennanConroy
Copy link
Member

Huh, I have no idea anymore, I get the hanging tests on master branch too on my Mac.

@dougbu
Copy link
Member Author

dougbu commented May 4, 2020

@BrennanConroy did you delete the .dotnet/ folder before trying again in 'master'? Some parts of the build seem to roll forward when multiple SDKs are available on dev machines. This probably means msbuild or dotnet processes are reused but I'm not sure.

@BrennanConroy
Copy link
Member

./clean.sh deletes .dotnet/ right?

@dougbu
Copy link
Member Author

dougbu commented May 4, 2020

Nope, -e .dotnet/ -e .tools/

@dougbu
Copy link
Member Author

dougbu commented May 4, 2020

git clean -dfx -e .tools/ would remove .dotnet/

@BrennanConroy
Copy link
Member

Oh wait, I did a manual rm -r .dotnet, I forgot

@dougbu
Copy link
Member Author

dougbu commented May 4, 2020

And, did you burn any leftover dotnet processes w/ fire or do the 'master' build w/ /nr:false?

@BrennanConroy
Copy link
Member

Yep (except for a zombie dotnet that just wont go away, but it's a zombie so it doesn't affect anything)

@dougbu
Copy link
Member Author

dougbu commented May 5, 2020

@BrennanConroy zombies 🧟 are not to be trifled with.

I get the hanging tests on master branch too on my Mac.

More seriously, what is your Mac doing exactly when the timeouts occur❔

And, because we're not seeing similar problems in the rolling builds of our 'master' branch, I wonder about scorched Earth approaches like clearing your NuGet caches and rebooting.

@dougbu
Copy link
Member Author

dougbu commented May 5, 2020

/fyi latest validation build (https://dev.azure.com/dnceng/public/_build/results?buildId=630538) showed some improvement. But,

Bottom line, the remaining test failures are specific to quarantined tests but they are also consistent enough to imply we may still be dealing with product issues such as the potential hostfxr bug @BrennanConroy mentioned earlier.

@BrennanConroy
Copy link
Member

* if `hostfxr` has indeed regressed

I'm doubting this is the case anymore

@dougbu
Copy link
Member Author

dougbu commented May 5, 2020

I'm doubting this is the case anymore

Any new and improved ideas❔

@BrennanConroy
Copy link
Member

Nope, my machine is unreliable since I can't even complete the tests on the master branch

@javiercn
Copy link
Member

javiercn commented May 5, 2020

@dougbu what makes you think the error has to do anything with the certificate? That error merely implies that the app is failing to start at all.

@dougbu
Copy link
Member Author

dougbu commented May 5, 2020

@javiercn the last server message before the client gives up is

The application is trying to access the ASP.NET Core developer certificate key. A prompt might appear to ask for permission to access the key. When that happens, select 'Always Allow' to grant 'dotnet' access to the certificate key in the future.

The server doesn't get around to emitting its usual "ready" messages. In addition, hasn't the notarization logic in the CLI and SDK changed since 5.0 preview 2?

@javiercn
Copy link
Member

javiercn commented May 5, 2020

@javiercn the last server message before the client gives up is

Yeah, sorry. I lost track of this PR and couldn't answer. I already looked into it and there was an issue with the cert loading code. I put out a fix and improved diagnostics and I'll merge it soon.

@javiercn
Copy link
Member

javiercn commented May 5, 2020

The server doesn't get around to emitting its usual "ready" messages. In addition, hasn't the notarization logic in the CLI and SDK changed since 5.0 preview 2?

Yes, that's why we are seeing this issue I think. The server is not picking up the certificate we generate I believe.

@BrennanConroy
Copy link
Member

I already looked into it and there was an issue with the cert loading code. I put out a fix and improved diagnostics and I'll merge it soon.

Your change is only for the Templates code, however some Kestrel tests are hanging with a similar issue it looks like. They are trying to access the dev cert and never completing. Were there recent changes to how the cert code works? Do we need to do some manual cert injection for the tests in Kestrel that use https?

@dougbu
Copy link
Member Author

dougbu commented May 5, 2020

/fyi I'll rerun the validation builds after #21493 and #21516 are in. Will handle the #21493 merge since @javiercn needs his sleep.

@dougbu
Copy link
Member Author

dougbu commented May 6, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@BrennanConroy
Copy link
Member

Yay, Mac passed 😄

@dougbu dougbu merged commit fdb0372 into master May 6, 2020
@dougbu dougbu deleted the dougbu/bump.sdk branch May 6, 2020 04:59
@ViktorHofer
Copy link
Member

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.