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.

Sign up for GitHub

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

Jump to bottom

Use PackageDownload in VS, fix design time build failures #3268

Merged
merged 8 commits into from May 29, 2019
Merged

Use PackageDownload in VS, fix design time build failures #3268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f2b6571
Use PackageDownload in full MSBuild if supported
dsplaisted May 25, 2019
da3db04
Don't fail design time builds if asset file doesn't have correct target
dsplaisted May 25, 2019
6e99b4a
Add tests for design time builds after project change
dsplaisted May 28, 2019
197617a
Use VS2019 CI queues
dsplaisted May 28, 2019
dcf2e55
Don't include DesignTimeBuild property in ResolvePackageAssets hash
dsplaisted May 29, 2019
a9e8d3a
Add dummy target for design time tests
dsplaisted May 29, 2019
834a02f
Disable PCL profile test which depends on Windows SDK
dsplaisted May 29, 2019
42ae8fc
Don't cache result of design time ResolvePackageAssets if assets file…
dsplaisted May 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions .vsts-ci.yml
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ jobs:
pool:
${{ if eq(variables['System.TeamProject'], 'public') }}:
name: NetCorePublic-Pool
queue: buildpool.windows.10.amd64.vs2017.open
queue: BuildPool.Windows.10.Amd64.VS2019.Pre.Open
${{ if ne(variables['System.TeamProject'], 'public') }}:
name: NetCoreInternal-Pool
queue: buildpool.windows.10.amd64.vs2017
Expand Down Expand Up @@ -75,7 +75,7 @@ jobs:
agentOs: Windows_NT_FullFramework
pool:
name: NetCorePublic-Pool
queue: buildpool.windows.10.amd64.vs2017.open
queue: BuildPool.Windows.10.Amd64.VS2019.Pre.Open
strategy:
matrix:
Build_Debug:
Expand All @@ -92,7 +92,7 @@ jobs:
agentOs: Windows_NT_TestAsTools
pool:
name: NetCorePublic-Pool
queue: buildpool.windows.10.amd64.vs2017.open
queue: BuildPool.Windows.10.Amd64.VS2019.Pre.Open
strategy:
matrix:
Build_Debug:
Expand Down Expand Up @@ -157,7 +157,7 @@ jobs:
agentOs: Windows_Perf_CI
pool:
name: NetCorePublic-Pool
queue: buildpool.windows.10.amd64.vs2017.open
queue: BuildPool.Windows.10.Amd64.VS2019.Pre.Open
strategy:
matrix:
Build_Release:
Expand Down
3 changes: 2 additions & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAResolvePackageAssetsTask.cs
View file
Open in desktop
Expand Up @@ -17,7 +17,8 @@ public void ItHashesAllParameters()
{
var inputProperties = typeof(ResolvePackageAssets)
.GetProperties(BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.Public)
.Where(p => !p.IsDefined(typeof(OutputAttribute)))
.Where(p => !p.IsDefined(typeof(OutputAttribute)) &&
p.Name != nameof(ResolvePackageAssets.DesignTimeBuild))
.OrderBy(p => p.Name, StringComparer.Ordinal);

var requiredProperties = inputProperties
Expand Down
100 changes: 79 additions & 21 deletions src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs
View file
Open in desktop
Expand Up @@ -147,6 +147,8 @@ public sealed class ResolvePackageAssets : TaskBase
[Required]
public string DotNetAppHostExecutableNameWithoutExtension { get; set; }

public bool DesignTimeBuild { get; set; }

/// <summary>
/// Full paths to assemblies from packages to pass to compiler as analyzers.
/// </summary>
Expand Down Expand Up @@ -371,6 +373,7 @@ internal byte[] HashSettings()
{
using (var writer = new BinaryWriter(stream, TextEncoding, leaveOpen: true))
{
writer.Write(DesignTimeBuild);
writer.Write(DisablePackageAssetsCache);
writer.Write(DisableFrameworkAssemblies);
writer.Write(DisableRuntimeTargets);
Expand Down Expand Up @@ -469,13 +472,12 @@ private static BinaryReader CreateReaderFromMemory(ResolvePackageAssets task, by
task.Log.LogMessage(MessageImportance.High, Strings.UnableToUsePackageAssetsCache);
}

var stream = new MemoryStream();
using (var writer = new CacheWriter(task, stream))
Stream stream;
using (var writer = new CacheWriter(task))
{
writer.Write();
stream = writer.WriteToMemoryStream();
}

stream.Position = 0;
return OpenCacheStream(stream, settingsHash);
}

Expand All @@ -499,10 +501,17 @@ private static BinaryReader CreateReaderFromDisk(ResolvePackageAssets task, byte
{
using (var writer = new CacheWriter(task))
{
writer.Write();
if (writer.CanWriteToCacheFile)
{
writer.WriteToCacheFile();
reader = OpenCacheFile(task.ProjectAssetsCacheFile, settingsHash);
}
else
{
var stream = writer.WriteToMemoryStream();
reader = OpenCacheStream(stream, settingsHash);
}
}

reader = OpenCacheFile(task.ProjectAssetsCacheFile, settingsHash);
}

return reader;
Expand Down Expand Up @@ -613,30 +622,69 @@ private sealed class CacheWriter : IDisposable
private NuGetFramework _targetFramework;
private int _itemCount;

public CacheWriter(ResolvePackageAssets task, Stream stream = null)
public bool CanWriteToCacheFile { get; set; }

public CacheWriter(ResolvePackageAssets task)
{
_targetFramework = NuGetUtils.ParseFrameworkName(task.TargetFrameworkMoniker);

_task = task;
_lockFile = new LockFileCache(task).GetLockFile(task.ProjectAssetsFile);
_packageResolver = NuGetPackageResolver.CreateResolver(_lockFile);
_compileTimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, runtime: null);
_runtimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, _task.RuntimeIdentifier);
_stringTable = new Dictionary<string, int>(InitialStringTableCapacity, StringComparer.Ordinal);
_metadataStrings = new List<string>(InitialStringTableCapacity);
_bufferedMetadata = new List<int>();
_platformPackageExclusions = GetPlatformPackageExclusions();

if (stream == null)

// If we are doing a design-time build, we do not want to fail the build if we can't find the
// target framework and/or runtime identifier in the assets file. This is because the design-time
// build needs to succeed in order to get the right information in order to run a restore in order
// to write the assets file with the correct information.

// So if we can't find the right target in the lock file and are doing a design-time build, we use
// an empty lock file target instead of throwing an error, and we don't save the results to the
// cache file.
CanWriteToCacheFile = true;
if (task.DesignTimeBuild)
{
Directory.CreateDirectory(Path.GetDirectoryName(task.ProjectAssetsCacheFile));
stream = File.Open(task.ProjectAssetsCacheFile, FileMode.Create, FileAccess.ReadWrite, FileShare.None);
_writer = new BinaryWriter(stream, TextEncoding, leaveOpen: false);
_compileTimeTarget = _lockFile.GetTarget(_targetFramework, runtimeIdentifier: null);
_runtimeTarget = _lockFile.GetTarget(_targetFramework, _task.RuntimeIdentifier);
if (_compileTimeTarget == null)
{
_compileTimeTarget = new LockFileTarget();
CanWriteToCacheFile = false;
}
if (_runtimeTarget == null)
{
_runtimeTarget = new LockFileTarget();
CanWriteToCacheFile = false;
}
}
else
{
_writer = new BinaryWriter(stream, TextEncoding, leaveOpen: true);
_compileTimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, runtime: null);
_runtimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, _task.RuntimeIdentifier);
}


_stringTable = new Dictionary<string, int>(InitialStringTableCapacity, StringComparer.Ordinal);
_metadataStrings = new List<string>(InitialStringTableCapacity);
_bufferedMetadata = new List<int>();
_platformPackageExclusions = GetPlatformPackageExclusions();
}

public void WriteToCacheFile()
{
Directory.CreateDirectory(Path.GetDirectoryName(_task.ProjectAssetsCacheFile));
var stream = File.Open(_task.ProjectAssetsCacheFile, FileMode.Create, FileAccess.ReadWrite, FileShare.None);
_writer = new BinaryWriter(stream, TextEncoding, leaveOpen: false);
Write();
}

public Stream WriteToMemoryStream()
{
var stream = new MemoryStream();
_writer = new BinaryWriter(stream, TextEncoding, leaveOpen: true);
Write();
stream.Position = 0;
return stream;
}

public void Dispose()
Expand All @@ -663,7 +711,7 @@ private void FlushMetadata()
_bufferedMetadata.Clear();
}

public void Write()
private void Write()
{
WriteHeader();
WriteItemGroups();
Expand Down Expand Up @@ -980,7 +1028,17 @@ private void WriteApphostsForShimRuntimeIdentifiers()

foreach (var runtimeIdentifier in _task.ShimRuntimeIdentifiers.Select(r => r.ItemSpec))
{
LockFileTarget runtimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, runtimeIdentifier);
bool throwIfAssetsFileTargetNotFound = !_task.DesignTimeBuild;
Copy link
Contributor

Choose a reason for hiding this comment

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

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...


LockFileTarget runtimeTarget;
if (_task.DesignTimeBuild)
{
runtimeTarget = _lockFile.GetTarget(_targetFramework, runtimeIdentifier) ?? new LockFileTarget();
}
else
{
runtimeTarget = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, runtimeIdentifier);
}

var apphostName = _task.DotNetAppHostExecutableNameWithoutExtension + ExecutableExtension.ForRuntimeIdentifier(runtimeIdentifier);

Expand Down
9 changes: 5 additions & 4 deletions ...Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.TargetingPackResolution.targets
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<UsingTask TaskName="ResolveFrameworkReferences" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />
<UsingTask TaskName="ResolveAppHosts" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />

<Target Name="ResolveFrameworkReferences" BeforeTargets="_CheckForInvalidConfigurationAndPlatform;CollectPackageReferences">
<Target Name="ResolveFrameworkReferences" BeforeTargets="_CheckForInvalidConfigurationAndPlatform;CollectPackageReferences;CollectPackageDownloads">

<CheckForDuplicateFrameworkReferences
FrameworkReferences="@(FrameworkReference)"
Expand Down Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

<UsePackageDownload Condition="'$(PackageDownloadSupported)' == 'true'">true</UsePackageDownload>
<UsePackageDownload Condition="'$(UsePackageDownload)' == ''">false</UsePackageDownload>
</PropertyGroup>

<ItemGroup Condition="'$(UsePackageDownload)' == 'true'">
Expand Down Expand Up @@ -283,7 +284,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<UsingTask TaskName="ResolveRuntimePackAssets" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />

<Target Name="ResolveRuntimePackAssets" DependsOnTargets="ResolvePackageAssets"
Condition="'@(RuntimePack)' != ''">
Condition="'@(RuntimePack)' != '' And '$(DesignTimeBuild)' != 'true'">

<GetPackageDirectory
Items="@(RuntimePack)"
Expand Down
6 changes: 4 additions & 2 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets
View file
Open in desktop
Expand Up @@ -177,7 +177,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<CheckForTargetInAssetsFile
AssetsFilePath="$(ProjectAssetsFile)"
TargetFrameworkMoniker="$(NuGetTargetMoniker)"
RuntimeIdentifier="$(RuntimeIdentifier)" />
RuntimeIdentifier="$(RuntimeIdentifier)"
Condition=" '$(DesignTimeBuild)' != 'true'"/>

<ResolvePackageDependencies
ProjectPath="$(MSBuildProjectFullPath)"
Expand Down Expand Up @@ -252,7 +253,8 @@ Copyright (c) .NET Foundation. All rights reserved.
EnsureRuntimePackageDependencies="$(EnsureRuntimePackageDependencies)"
VerifyMatchingImplicitPackageVersion="$(VerifyMatchingImplicitPackageVersion)"
ExpectedPlatformPackages="@(ExpectedPlatformPackages)"
SatelliteResourceLanguages="$(SatelliteResourceLanguages)">
SatelliteResourceLanguages="$(SatelliteResourceLanguages)"
DesignTimeBuild="$(DesignTimeBuild)">

<!-- NOTE: items names here are inconsistent because they match prior implementation
(that was spread across different tasks/targets) for backwards compatibility. -->
Expand Down