Use PackageDownload in VS, fix design time build failures #3268
Conversation
dsplaisted
commented
May 26, 2019
dsplaisted
commented
- Use PackageDownload when supported on full MSBuild. Fixes https://github.com/dotnet/cli/issues/10440
- Don't fail design-time builds when the target framework or RuntimeIdentifier doesn't match what's in the (now outdated) assets file. Fixes [Feedback] We should not be breaking design-time builds when the assets file is missing framework data #2322
- I'd like to add tests to cover this, but haven't yet
dsplaisted
force-pushed
the
use-packagedownload2
branch
from
4acf800
to
6984525
Compare
|
Yes, I tested this with VS, and ran into some problems which ended up being #2322. With that fixed, it now appears to work great in VS. |
| @@ -12,28 +12,36 @@ namespace Microsoft.NET.Build.Tasks | |||
| { | |||
| internal static class LockFileExtensions | |||
| { | |||
| public static LockFileTarget GetTargetAndThrowIfNotFound(this LockFile lockFile, NuGetFramework framework, string runtime) | |||
| public static LockFileTarget GetTargetAndThrowIfNotFound(this LockFile lockFile, NuGetFramework framework, string runtime, | |||
| bool throwIfNotFound = true) | |||
There was a problem hiding this comment.
The API shape here is getting weird. AndThrowIfNotFound vs. bool throwIfNotFound
Having trouble coming up with a better shape though. Feels like it should just be GetTarget with the parameters modifying the behavior, but that conflicts with real instance GetTarget that can return null.
| @@ -980,7 +987,10 @@ private void WriteApphostsForShimRuntimeIdentifiers() | |||
|
|
|||
| foreach (var runtimeIdentifier in _task.ShimRuntimeIdentifiers.Select(r => r.ItemSpec)) | |||
| { | |||
| LockFileTarget runtimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, runtimeIdentifier); | |||
| bool throwIfAssetsFileTargetNotFound = !_task.DesignTimeBuild; | |||
There was a problem hiding this comment.
Here, the bool local feels like it's unnecessary. Would be easier to read (subjectively) as `throwIfNotFound: !_task.DesignTimeBuild. Earlier, though, the local is used more than once so it has some value, and then I can see wanting to be consistent between the methods. So I'm not sure...
| @@ -142,8 +142,9 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| </ResolveAppHosts> | |||
|
|
|||
| <PropertyGroup Condition="'$(UsePackageDownload)' == ''"> | |||
| <UsePackageDownload Condition=" '$(MSBuildRuntimeType)' == 'Core'">true</UsePackageDownload> | |||
| <UsePackageDownload Condition=" '$(MSBuildRuntimeType)' != 'Core'">false</UsePackageDownload> | |||
| <UsePackageDownload Condition="'$(MSBuildRuntimeType)' == 'Core'">true</UsePackageDownload> | |||
There was a problem hiding this comment.
Do we need this anymore? I can't think of a case from here forward where we'd be using core msbuild but PackageDownloadSupported != true. If we don't need it now, might as well keep it out. I guess this whole thing will go away. but I think there's still some value in keeping the bridge code as small as possible until then. Less moving parts to think about.
|
Design time tests are now failing in CI because I updated them to run all the targets that ran on a design time build on my machine, and the version of VS used by the CI machines doesn't have a recent version of VS that defines all of those targets. |
| @@ -230,6 +231,15 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <ExpectedPlatformPackages Include="@(PackageReference)" Condition="'%(Identity)' == 'Microsoft.AspNetCore.All'" /> | |||
| </ItemGroup> | |||
|
|
|||
| <CheckForTargetInAssetsFile | |||
There was a problem hiding this comment.
I think this will be a perf regression, forcing the assets file to be read from disk on every incremental build. I think it used be like this in fact, and I folded it into ResolvePackageAssets for this reason.
This should fix dotnet#2322. The issue is there could be a catch-22 where there is an assets file with the wrong targets (because the RuntimeIdentifier or TargetFramework has changed) and we need to do a design-time build to nominate restore (which will write the right targets to the assets file), but the build to get the nomination data fails because the assets file has the wrong targets.
dsplaisted
force-pushed
the
use-packagedownload2
branch
from
99550dc
to
dcf2e55
Compare
dsplaisted
force-pushed
the
use-packagedownload2
branch
from
62fa035
to
834a02f
Compare
… doesn't have correct target
….2 (dotnet#3268) - Microsoft.DotNet.Cli.Runtime - 3.1.100-preview2.19522.2
