use inclusion list to add DropFromSingleFile attribute by VSadov · Pull Request #5440 · dotnet/arcade

VSadov · 2020-05-08T05:10:00Z

Another take at "mark native files in RuntimeList.xml as DropFromSingleFile unless from a specific list of known files"
see previous attempt at dotnet/runtime#36047

This time the code that sets the attributes is going to be in Arcade, while the inclusion list will be passed by runtime build, so that the feature could be turned on and configured without further changes on Arcade side.

This whole deal is a step towards dotnet/sdk#11567

swaroop-sridhar · 2020-05-08T05:20:12Z


        public string[] TargetFilePrefixes { get; set; }

+        public string[] SingleFileHostIncludeList { get; set; }


I think it is a good idea you made it an IncludeList, rather than an ExcludeList.
New native components may be included in the package, but they are unlikely to be necessary for the single-file case.

Yes. The inclusion list seems to be just a few well known files. Most files are excluded, so I figured that should be the default behavior.

Also note that if the list is not specified then we have preexisting behavior. This is mostly so that we would not start excluding everything right away.

If we want, we may make the list a required property in later changes.

dagood · 2020-05-08T05:39:37Z


        public string[] TargetFilePrefixes { get; set; }

+        public string[] SingleFileHostIncludeList { get; set; }


Why a separate list, rather than using metadata on Files?

Files come from various places and often without explicitly listing actual files (just include whole folders for example). We would need to implement conditional adding of metadata - either in multiple places or in one central place just before invoking the task.
But then it becomes easier to pass the list into the task and do filtering in there.

These files are known parts of the runtime and the list is short (21 files between all platforms currently). It is more manageable to just list them in one place.

I agree that the meta-data option is a cleaner interface if it can be realized in a straightforward manner (ex: in one place in runtime repo before invoking this task).

It is a bit more generic, if we need to add other attributes to the list (rather than creating a new list for each).

It is also closer to the way ultimately the list will be used in the SDK -- transformation into meta-data on the file-to-copy list.

I started initially on that path. What it leads to, is essentially a list of conditions in msbuild - so that we could add metadata to File items based on their identities. That is all so that the task will immediately transform the metadata to attributes.

Maybe there is a better way to do this, I am not an expert in msbuild, but it looked like the approach was unnecessary complex and probably expensive, without any advantages compared to just passing the list down to the task.
I.E. if we want to add another kind of metadata, we would have another list of conditional items and another piece of code in the task that transforms them into attributes.

It started looking like the following:

<Error Condition="'%(FrameworkListRootAttributes.Value)' == ''" Text="Missing value for property 'FrameworkList%(FrameworkListRootAttributes.Identity)'" /> <ItemGroup> <File Condition="'%(File.IsNative)'=='true' AND '%(File.Identity)'!='libSystem.Globalization.Native.so' AND '%(File.Identity)'!='libSystem.IO.Compression.Native.so' AND '%(File.Identity)'!='libSystem.IO.Ports.Native.so' AND '%(File.Identity)'!='libSystem.Native.so' AND '%(File.Identity)'!='libSystem.Net.Security.Native.so' AND '%(File.Identity)'!='llibSystem.Security.Cryptography.Native.OpenSsl.so' AND '%(File.Identity)'!='libclrjit.so' AND '%(File.Identity)'!='libcoreclr.so' AND '%(File.Identity)'!='libSystem.Globalization.Native.dylib' AND '%(File.Identity)'!='libSystem.IO.Compression.Native.dylib' AND '%(File.Identity)'!='libSystem.IO.Ports.Native.dylib' AND '%(File.Identity)'!='libSystem.Native.dylib' AND '%(File.Identity)'!='libSystem.Net.Security.Native.dylib' AND '%(File.Identity)'!='libSystem.Security.Cryptography.Native.Apple.dylib' AND '%(File.Identity)'!='libSystem.Security.Cryptography.Native.OpenSsl.dylib' AND '%(File.Identity)'!='libclrjit.dylib' AND '%(File.Identity)'!='libcoreclr.dylib' AND '%(File.Identity)'!='clrcompression.dll' AND '%(File.Identity)'!='clrjit.dll' AND '%(File.Identity)'!='coreclr.dll' AND '%(File.Identity)'!='mscordaccore.dll' "> <DropFromSingleFile>true</DropFromSingleFile> </File> </ItemGroup> <CreateFrameworkListFile Files="@(File)" FileClassifications="@(FrameworkListFileClass)" TargetFile="$(FrameworkListFile)" TargetFilePrefixes="ref/;runtimes/" RootAttributes="@(FrameworkListRootAttributes)" />

Yes, that is the same logic, except filtering via a hashset is probably more efficient.
I was mostly concerned about gigantic conditional that seemed unnatural and a bit fragile.

Your example seems more tidy than mine. I did not realize such trick could be used. I think we can use that.

I don't think that works... I think you mean Remove rather than Exclude (Exclude just limits an Include on the same item element), but with this test project:

<Project> <ItemGroup> <A Include="a\b\c\a.txt" /> <A Include="a\b\c\b.txt" /> <A Include="a\b\c\c.txt" /> <A Include="a\b\c\d.txt" /> <A Include="a\b\c\e.txt" /> <B Include="b.txt" /> <B Include="d.txt" /> </ItemGroup> <Target Name="Build"> <ItemGroup> <A Remove="@(B)" /> <A Include="@(B)" DropFromSingleFile="true" /> <Results Include="@(A)" /> </ItemGroup> </Target> </Project>

You get this result (binlog tree):

Target Name=Build Project=build.proj A b.txt DropFromSingleFile = true d.txt DropFromSingleFile = true Results a\b\c\a.txt a\b\c\b.txt a\b\c\c.txt a\b\c\d.txt a\b\c\e.txt b.txt DropFromSingleFile = true d.txt DropFromSingleFile = true

This is because these are different files--the exclude/remove doesn't only take into account filename.

You can work around this with an inversion trick:

<Project> <ItemGroup> <A Include="a\b\c\a.txt" /> <A Include="a\b\c\b.txt" /> <A Include="a\b\c\c.txt" /> <A Include="a\b\c\d.txt" /> <A Include="a\b\c\e.txt" /> <B Include="b.txt" /> <B Include="d.txt" /> </ItemGroup> <Target Name="Build"> <ItemGroup> <AFilenames Include="@(A -> '%(Filename)%(Extension)')" OriginalIdentity="%(Identity)" /> <AFilenamesNotInB Include="@(AFilenames)" Exclude="@(B)" /> <AFilenamesInB Include="@(AFilenames)" Exclude="@(AFilenamesNotInB)" IncludeInSingleFile="true" /> <Results Include=" @(AFilenamesNotInB -> '%(OriginalIdentity)'); @(AFilenamesInB -> '%(OriginalIdentity)')" /> </ItemGroup> </Target> </Project>

Results a\b\c\a.txt OriginalIdentity = a\b\c\a.txt a\b\c\c.txt OriginalIdentity = a\b\c\c.txt a\b\c\e.txt OriginalIdentity = a\b\c\e.txt a\b\c\b.txt IncludeInSingleFile = true OriginalIdentity = a\b\c\b.txt a\b\c\d.txt IncludeInSingleFile = true OriginalIdentity = a\b\c\d.txt

However what matters here IMO is clarity and maintainability. I don't think performance and MSBuildiness are problems except when things get extreme.

So I do think what you have here is fine, just with the naming tweak I suggested.

(And in general, having a quick setup to try out MSBuild stuff in isolation is incredibly useful when working on stuff like this.)

Using full filenames would be good (and make it work in the simple way), but I assume that's infeasible because:

Files come from various places and often without explicitly listing actual files (just include whole folders for example).

So I do think what you have here is fine

I am confused - which one is fine? The one where list is passed to the task or where metadata is used?

The list of filenames, what it was when I posted #5440 (comment).

dagood · 2020-05-08T17:22:34Z


        public string[] TargetFilePrefixes { get; set; }

+        public string[] SingleFileHostIncludeList { get; set; }


Improve the name of this property? (Normally want to describe the contents, not that it's a list.)

Suggested change

public string[] SingleFileHostIncludeList { get; set; }

public string[] SingleFileHostIncludeFilenames { get; set; }

VSadov · 2020-05-08T21:28:14Z

in the light of needing to split paths and names (which we already do in the task), let's pass the list as an argument to the task.
Also addressed naming concerns.

VSadov · 2020-05-08T23:06:37Z

Is this ok to merge?

VSadov · 2020-05-10T03:26:12Z

Thanks!!

VSadov · 2020-05-10T20:49:01Z

How soon could this change be in runtime repo? Is that automatic?

dagood · 2020-05-10T22:02:13Z

It looks like the Arcade folks are keeping track at https://github.com/dotnet/arcade#validation--dependency-flow-status. It's automatic in terms of dotnet/runtime getting an auto-PR sometime in the future.

VSadov · 2020-05-10T22:53:56Z

Hmm. I kind of expected bots will flow the changes in a few hours.
Can we ask somebody for the the auto-PR?

dagood · 2020-05-11T05:14:11Z

Not sure. I haven't followed an arcade change in a while, I just know I've been getting emails about more manual stuff going on, and messages along those lines in the Arcade Teams channel. Worth asking, and IMO also asking them to improve the docs where there isn't clarity here.

markwilkie · 2020-05-11T14:09:06Z

The new(ish) policy is here: https://github.com/dotnet/arcade/blob/master/Documentation/Validation/Overview.md and as Davis referenced, the latest status is https://github.com/dotnet/arcade#validation--dependency-flow-status.

In an effort to improve docs (as you say Davis), what could be better do you think? Perhaps x-link the policy and status?

@VSadov - unfortunately, given the current instability of build .net builds, we can't auto-flow updates. Once the product calms down some, then we can certainly visit that.

An an FYI - assuming the repos are green (according to the policy), we plan to promote Arcade today.

dagood · 2020-05-11T14:40:07Z

In an effort to improve docs (as you say Davis), what could be better do you think? Perhaps x-link the policy and status?

Yeah, the docs folder is pretty impenetrable at the moment (tons of docs, where many but not all are not in a structure; no guiding indices/readmes), so other than searching my email I don't know how to get there. This particular link is definitely missing.

I didn't do a lot of digging though, @VSadov may have more direct feedback.

markwilkie · 2020-05-11T14:59:49Z

It is indeed a bit of a jungle. :(

dotnet/dnceng#1179

use inclusion list to add DropFromSingleFile attribute

9e5d838

VSadov requested review from dagood and swaroop-sridhar May 8, 2020 05:10

swaroop-sridhar reviewed May 8, 2020

View reviewed changes

swaroop-sridhar approved these changes May 8, 2020

View reviewed changes

dagood reviewed May 8, 2020

View reviewed changes

dagood requested a review from NikolaMilosavljevic May 8, 2020 05:43

dagood reviewed May 8, 2020

View reviewed changes

PR feedback - SingleFileHostIncludeFilenames

56bb876

VSadov force-pushed the DropFromSingleFile branch from 0a5ac64 to 56bb876 Compare May 8, 2020 21:22

dagood approved these changes May 10, 2020

View reviewed changes

VSadov merged commit c6a0476 into dotnet:master May 10, 2020

VSadov deleted the DropFromSingleFile branch May 10, 2020 03:25


		public string[] TargetFilePrefixes { get; set; }

		public string[] SingleFileHostIncludeList { get; set; }

	public string[] SingleFileHostIncludeList { get; set; }
	public string[] SingleFileHostIncludeFilenames { get; set; }

Conversation

VSadov commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov May 8, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagood May 8, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented May 8, 2020

Uh oh!

VSadov commented May 8, 2020

Uh oh!

VSadov commented May 10, 2020

Uh oh!

VSadov commented May 10, 2020

Uh oh!

dagood commented May 10, 2020

Uh oh!

VSadov commented May 10, 2020

Uh oh!

dagood commented May 11, 2020

Uh oh!

markwilkie commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagood commented May 11, 2020

Uh oh!

markwilkie commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VSadov commented May 8, 2020 •

edited

Loading

VSadov May 8, 2020 •

edited

Loading

dagood May 8, 2020 •

edited

Loading

markwilkie commented May 11, 2020 •

edited

Loading