-
Notifications
You must be signed in to change notification settings - Fork 82
Add a task to generate a manifest with the file system structure of the original files #308
Conversation
f3c8e22
to
cd4eeba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good start, but needs some refinement.
I'm assuming the runtime consumer will come in a separate PR?
<AssemblyName>Microsoft.Extensions.FileProviders.Embedded.Manifest.Task</AssemblyName> | ||
<Description>MSBuild task to generate a manifest that can be used by Microsoft.Extensions.FileProviders.Embedded to preserve | ||
metadata of the files embedded in the assembly at compilation time.</Description> | ||
<TargetFramework>netstandard2.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this when building with MSBuild.exe
from a VS developer command prompt, or when building inside VS? In the past, we found it better to cross-target task assemblies to .NET Framework because MSBuild for VS didn't include ns2.0 binding redirects until 15.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building on VS worked just fine and I don't think the dependency on 15.4 will be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we expect users to have 15.4 or newer, we should find a way to encode that expectation, such as setting the minimum nuget client version that corresponds to VS 15.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll discuss this offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new packages layout will take care of this dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to target .NET Framework. The package that uses this task would install in a project that could be as old as MSBuild 15.3. If you don't, VS users not on latest will hit TypeLoadException errors when they try to compile their app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example
<PropertyGroup>
<TaskAssembly Condition=" '$(MSBuildRuntimeType)' == 'Core'">.\bin\Debug\netstandard1.6\GreetingTasks.dll</TaskAssembly>
<TaskAssembly Condition=" '$(MSBuildRuntimeType)' != 'Core'">.\bin\Debug\net46\GreetingTasks.dll</TaskAssembly>
</PropertyGroup>
<UsingTask TaskName="MSBuildTasks.SayHello" AssemblyFile="$(TaskAssembly)" />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<RootNamespace>Microsoft.Extensions.FileProviders.Embedded.Manifest.Task</RootNamespace> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered putting this inside the Microsoft.Extensions.FileProviders.Embedded nuget package, instead of making a new package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the MSBuild part of it? (The targets file). I would definitely keep the assembly separate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assembly definitely needs to be separate. One way to layout this package is like this:
- Microsoft.Extensions.FileProviders.Embedded.nupkg
- lib/
- netstandard2.0/
+ Microsoft.Extensions.FileProviders.Embedded.dll
- build/
- netstandard2.0/
+ Microsoft.Extensions.FileProviders.Embedded.targets
+ Microsoft.Extensions.FileProviders.Embedded.Manifest.Task.dll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
test/Directory.Build.props
Outdated
@@ -1,4 +1,4 @@ | |||
<Project> | |||
<Project sdk="Microsoft.NET.Sdk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't put this here. This belongs in the csproj file, not .props files. This can change the order of a target imports.
namespace Microsoft.Extensions.FileProviders.Embedded.Manifest.Task | ||
{ | ||
[DebuggerDisplay("{Name,nq}")] | ||
public class Entry : IEquatable<Entry> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can be internal
since this isn't API we want anyone using directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it on the internal namespace, but I use this all over in testing and don't want to do internalsvisibleto dance.
<__ManifestFiles Include="@(EmbeddedResource->WithMetadataValue('IsManifest', 'true'))" /> | ||
</ItemGroup> | ||
|
||
<Error Condition="@(__ManifestFiles->Count()) > 1" Text="Multiple manifest files found. Only one manifest file per assembly is allowed: %(__ManifestFiles.TargetPath)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who sets TargetPath and how? Normally, any target that generates intermediate files should put them into $(IntermediateOutputPath)
(which defaults to obj/$Configuration/$TargetFramework)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Target path is set by the default manifest above to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to use @(__ManifestFiles)
. The current approach would stop after printing the first item.
<PropertyGroup> | ||
<TargetFramework>netcoreapp2.1</TargetFramework> | ||
<RuntimeFrameworkVersion>2.1.0-preview2-25624-02</RuntimeFrameworkVersion> | ||
<NETStandardImplicitPackageVersion>2.1.0-preview2-25624-02</NETStandardImplicitPackageVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise: can be removed
["LogicalName"] = logicalName ?? targetPath.Replace("/", ".").Replace("\\", "."), | ||
}; | ||
|
||
private class TestTaskItem : ITaskItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use Microsoft.Build.Utilities.TaskItem, which implements ITaskItem
LogicalName="$(EmbeddedFilesManifestFileName)" /> | ||
</ItemGroup> | ||
|
||
<Target Name="GenerateEmbeddedFilesManifest" AfterTargets="PrepareResourceNames" Condition="$(EmbeddedFilesManifest) == 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break incremental compilation. You need to ensure you never re-write the manifest file unless the list of EmbeddedResource's changed. Typically this is done by specifying Inputs
and Outputs
on a target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the supreme MSBuild incantator, can you tell me how to do this? The manifest is already order resilient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to have targets that are implementation details (private) to start with an underscore. Consider splitting this off in to two targets
<Target Name="_GenerateEmbeddedFilesManifest" AfterTargets="PrepareResourceNames" DependsOnTargets="GenerateEmbeddedFilesManifest" Condition="$(EmbeddedFilesManifest) == 'true'" />
<Target Name="GenerateEmbeddedFilesManifest">
...
</Target>
so a user could wire up the second one.
public ITaskItem[] EmbeddedFiles { get; set; } | ||
|
||
[Required] | ||
public ITaskItem ManifestFile { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the only usage of this is to acquire the TargetPath
metadata item, right? Any reason this couldn't just be public string ManifestFilePath { get; set; }
?
|
||
namespace Microsoft.Extensions.FileProviders.Embedded.Manifest.Task | ||
{ | ||
public class GenerateEmbeddedResourcesManifestTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 seems like good test coverage.
throw new InvalidOperationException("Tried to add two different items with the same name"); | ||
} | ||
|
||
Children.Add(child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have a check for a situation, where the child has already been added, I would throw in that case also, instead of silently ignoring the request
|
||
private class NameComparer : IComparer<Entry> | ||
{ | ||
public static NameComparer Instance { get; set; } = new NameComparer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the "set" be "private" to avoid reassignments?
ManifestFilePath = ManifestFile.GetMetadata(TargetPath) | ||
}; | ||
|
||
var document = manifest.ToXmlDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this inside the using scope
AssemblyResourceName = GetAssemblyResourceName(er) | ||
}).ToArray(); | ||
|
||
string GetManifestPath(ITaskItem er) => string.Equals(er.GetMetadata(LogicalName), er.GetMetadata(ManifestResourceName)) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide explicit visibility keywords for those methods
string NormalizePath(string path) => Path.DirectorySeparatorChar == '\\' ? | ||
path.Replace("/", "\\") : path.Replace("\\", "/"); | ||
|
||
public Manifest BuildManifest(EmbeddedItem[] processedItems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate the "processedItems" to be not null before usage
|
||
public Entry Root { get; set; } = Entry.Directory(""); | ||
|
||
public void AddElement(string originalPath, string assemblyResourceName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please validate arguments for all public methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please resolve.
Addressed all feedback /cc: @natemcmaster @mkArtakMSFT |
private string NormalizePath(string path) => Path.DirectorySeparatorChar == '\\' ? | ||
path.Replace("/", "\\") : path.Replace("\\", "/"); | ||
|
||
public Manifest BuildManifest(EmbeddedItem[] processedItems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: would be nice to have all the public methods before private ones (or private methods in the bottom)
|
||
public void AddElement(string originalPath, string assemblyResourceName) | ||
{ | ||
var paths = originalPath.Split(Path.DirectorySeparatorChar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing argument validation here. originalPath can be null and that'll result in NullReferenceException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more pending comments
} | ||
|
||
public EmbeddedItem[] CreateEmbeddedItems(params ITaskItem[] items) => | ||
items.Select(er => new EmbeddedItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null check. Why is this method public? Why a params
array?
er.GetMetadata(TargetPath) : | ||
NormalizePath(er.GetMetadata(LogicalName)); | ||
|
||
private string GetAssemblyResourceName(ITaskItem er) => string.Equals(er.GetMetadata(LogicalName), er.GetMetadata(ManifestResourceName)) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ITaskItem taskItem
TaskName="Microsoft.Extensions.FileProviders.Embedded.Manifest.Task.GenerateEmbeddedResourcesManifest" /> | ||
|
||
<PropertyGroup> | ||
<EmbeddedFilesManifest Condition="$(EmbeddedFilesManifest) == ''">false</EmbeddedFilesManifest> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Condition="'$(EmbeddedFilesManifest)' == ''"
. Use single quotes to wrap your variables in conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this start with a verb - e.g. GenerateEmbeddedFilesManifest
?
LogicalName="$(EmbeddedFilesManifestFileName)" /> | ||
</ItemGroup> | ||
|
||
<Target Name="GenerateEmbeddedFilesManifest" AfterTargets="PrepareResourceNames" Condition="$(EmbeddedFilesManifest) == 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to have targets that are implementation details (private) to start with an underscore. Consider splitting this off in to two targets
<Target Name="_GenerateEmbeddedFilesManifest" AfterTargets="PrepareResourceNames" DependsOnTargets="GenerateEmbeddedFilesManifest" Condition="$(EmbeddedFilesManifest) == 'true'" />
<Target Name="GenerateEmbeddedFilesManifest">
...
</Target>
so a user could wire up the second one.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>netcoreapp2.1</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Desktop?
<__ManifestFiles Include="@(EmbeddedResource->WithMetadataValue('IsManifest', 'true'))" /> | ||
</ItemGroup> | ||
|
||
<Error Condition="@(__ManifestFiles->Count()) > 1" Text="Multiple manifest files found. Only one manifest file per assembly is allowed: %(__ManifestFiles.TargetPath)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to use @(__ManifestFiles)
. The current approach would stop after printing the first item.
Name="GenerateEmbeddedFilesManifest" | ||
AfterTargets="PrepareResourceNames" | ||
Condition="$(EmbeddedFilesManifest) == 'true'" | ||
Inputs="@(EmbeddedResource)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this is going to run in to dotnet/msbuild#2470. csc works around this by manually tracking file hashes - https://github.com/Microsoft/msbuild/pull/1328/files
return string.Equals(Name, currentSegment, StringComparison.Ordinal); | ||
} | ||
|
||
public bool Equals(Entry other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetHashCode
\ Equals(object)
overrides?
|
||
private bool SameChildren(ISet<Entry> left, ISet<Entry> right) | ||
{ | ||
if (left.Count != right.Count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a SetEquals on set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equality for these sets is not based solely on the name. We check that we aren't adding two files with the same name but different metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. But you should be able to use ``SortedSet.CreateSetComparer(EqualityComparer.Default).Equals(left, right);`
AssemblyResourceName = GetAssemblyResourceName(er) | ||
}).ToArray(); | ||
|
||
private string GetManifestPath(ITaskItem er) => string.Equals(er.GetMetadata(LogicalName), er.GetMetadata(ManifestResourceName)) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gross
Name="GenerateEmbeddedFilesManifest" | ||
AfterTargets="PrepareResourceNames" | ||
Condition="$(EmbeddedFilesManifest) == 'true'" | ||
Inputs="@(EmbeddedResource)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start. You need a few additional things to make this work correctly. Inputs/Outputs only compares the timestamps of each file and asserts that Inputs.Max(f => f.LastModifiedTime) < Outputs.Min(f =>f .LastModifiedTime)
. What it doesn't do is compare if the list of Inputs and Outputs changes between runs. So if I remove an embedded resource, the assertion still checks out, and the manifest isn't re-built, but the list of embedded files is wrong.
The way we've worked around this in dotnet/sdk is to add an additional input, which generates a hash of all items and relevant metadata.
For a good example of this, checkout the assembly attribute generation targets: https://github.com/dotnet/sdk/blob/2995c8495082d1dd4bcad35c37007375edf45c6f/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.GenerateAssemblyInfo.targets#L114
<!--
To allow version changes to be respected on incremental builds (e.g. through CLI parameters),
create a hash of all assembly attributes so that the cache file will change with the calculated
assembly attribute values and msbuild will then execute CoreGenerateAssembly to generate a new file.
-->
<Target Name="CreateGeneratedAssemblyInfoInputsCacheFile"
DependsOnTargets="GetAssemblyAttributes">
<PropertyGroup>
<GeneratedAssemblyInfoInputsCacheFile>$(IntermediateOutputPath)$(MSBuildProjectName).AssemblyInfoInputs.cache</GeneratedAssemblyInfoInputsCacheFile>
</PropertyGroup>
<!-- We only use up to _Parameter1 for most attributes, but other targets may add additional assembly attributes with multiple parameters. -->
<Hash ItemsToHash="@(AssemblyAttribute->'%(Identity)%(_Parameter1)%(_Parameter2)%(_Parameter3)%(_Parameter4)%(_Parameter5)%(_Parameter6)%(_Parameter7)%(_Parameter8)')">
<Output TaskParameter="HashResult" PropertyName="_AssemblyAttributesHash" />
</Hash>
<WriteLinesToFile Lines="$(_AssemblyAttributesHash)" File="$(GeneratedAssemblyInfoInputsCacheFile)" Overwrite="True" WriteOnlyWhenDifferent="True" />
<ItemGroup>
<FileWrites Include="$(GeneratedAssemblyInfoInputsCacheFile)" />
</ItemGroup>
</Target>
<Target Name="CoreGenerateAssemblyInfo"
Condition="'$(Language)'=='VB' or '$(Language)'=='C#'"
DependsOnTargets="CreateGeneratedAssemblyInfoInputsCacheFile"
Inputs="$(GeneratedAssemblyInfoInputsCacheFile)"
Outputs="$(GeneratedAssemblyInfoFile)">
<ItemGroup>
<!-- Ensure the generated assemblyinfo file is not already part of the Compile sources, as a workaround for https://github.com/dotnet/sdk/issues/114 -->
<Compile Remove="$(GeneratedAssemblyInfoFile)" />
</ItemGroup>
<WriteCodeFragment AssemblyAttributes="@(AssemblyAttribute)" Language="$(Language)" OutputFile="$(GeneratedAssemblyInfoFile)">
<Output TaskParameter="OutputFile" ItemName="Compile" />
<Output TaskParameter="OutputFile" ItemName="FileWrites" />
</WriteCodeFragment>
</Target>
a17e058
to
192b00b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When @natemcmaster is happy
|
||
private bool SameChildren(ISet<Entry> left, ISet<Entry> right) | ||
{ | ||
if (left.Count != right.Count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. But you should be able to use ``SortedSet.CreateSetComparer(EqualityComparer.Default).Equals(left, right);`
<Target Name="CreateGeneratedManifestInfoInputsCacheFile"> | ||
<PropertyGroup> | ||
<GeneratedManifestInfoInputsCacheFile>$(IntermediateOutputPath)$(MSBuildProjectName).EmbeddedFilesManifest.cache</GeneratedManifestInfoInputsCacheFile> | ||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting
Condition="@(__FilesForManifest->Count()) != 0" /> | ||
|
||
</Target> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line at eof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to cross-target the task assembly for .NET Framework.
Incremental compilation code looks good, but we should add tests to assert this works. This is a critical scenario that we shouldn't break.
] | ||
], | ||
"packages": { | ||
"Microsoft.Extensions.FileProviders.Embedded.Manifest.Task": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this now.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<RootNamespace>Microsoft.Extensions.FileProviders.Embedded.Manifest.Task</RootNamespace> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
<AssemblyName>Microsoft.Extensions.FileProviders.Embedded.Manifest.Task</AssemblyName> | ||
<Description>MSBuild task to generate a manifest that can be used by Microsoft.Extensions.FileProviders.Embedded to preserve | ||
metadata of the files embedded in the assembly at compilation time.</Description> | ||
<TargetFramework>netstandard2.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to target .NET Framework. The package that uses this task would install in a project that could be as old as MSBuild 15.3. If you don't, VS users not on latest will hit TypeLoadException errors when they try to compile their app.
<Project> | ||
|
||
<UsingTask | ||
AssemblyFile="Microsoft.Extensions.FileSystem.Embedded.Manifest.Task" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be a file path
<__ManifestFiles Include="@(EmbeddedResource->WithMetadataValue('IsManifest', 'true'))" /> | ||
</ItemGroup> | ||
|
||
<Error Condition="'@(__ManifestFiles->Count())' > 1" Text="Multiple manifest files found. Only one manifest file per assembly is allowed: @(__ManifestFiles->TargetPath)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@(__ManifestFiles->TargetPath)
is this valid MSBuild syntax? I don't recognized it.
<EmbeddedResource | ||
Include="$(IntermediateOutputPath)$(EmbeddedFilesManifestFileName)" | ||
IsManifest="true" | ||
Condition="@(EmbeddedResource->WithMetadataValue('IsManifest','true')->Count()) == 0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure a users would ever need to write their own manifest. Seems better to always let our targets perform the manifest generation.
But if this is an important scenario, you should add a functional test that ensures this scenario works. From what I can tell, the GenerateEmbeddedResourcesManifest task would end up overwriting their file with generated code.
Example automated test for asserting incremental compliation: https://github.com/dotnet/sdk/blob/73fb22a07091585380ead0bcbcdc38839a0266e8/test/Microsoft.NET.Build.Tests/GivenThatWeWantToControlGeneratedAssemblyInfo.cs#L122-L194 |
I've fix a bug for adding tests for incremental compilation and I will be setting up a meeting to discuss what approach we want to take here. #309 |
this feature's gonna be dope. |
5a6cabe
to
a2becdf
Compare
* Generates an XML manifest respecting the original structure of the file system at the time the files were embedded in the assembly. * `<GenerateEmbeddedFilesManifest>true</GenerateEmbeddedFilesManifest>` enables the feature. * Supports changing the manifest name with the EmbeddedFilesManifestFileName property. * Supports excluding specific files from the manifest using the attribute ExcludeFromManifest=true on the EmbeddedResource item. * Supports changing the path to the file using the LogicalName property on the embedded resource.
a2becdf
to
d5998f1
Compare
No description provided.