Cache Embedded Resources during compilation task #6780
Conversation
| @@ -3672,6 +3672,7 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||
| <ItemGroup> | |||
| <CustomAdditionalCompileInputs Include="$(IntermediateOutputPath)$(MSBuildProjectFile).CoreCompileInputs.cache" /> | |||
| <CoreCompileCache Include="@(Compile)" /> | |||
| <CoreCompileCache Include="@(EmbeddedResource)" /> | |||
There was a problem hiding this comment.
@(EmbeddedResource) is not passed to the compiler so it's not the right item here. It looks like @(_CoreCompileResourceInputs) would be the right item:
That's defined here:
msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets
Lines 3503 to 3505 in 46d8f9b
So this PR should probably take a dependency on that target too.
There was a problem hiding this comment.
I see, so we extract some outputs of EmbeddedResource items based on whether they are Resx or not without Culture! Only those are passed into CoreCompile, the rest is ignored.
If that's the case, then, shouldn't we do the same for ReferencePath? Here, the ReferencePathWithRefAssemblies items are the final input to the CoreCompile, not ReferencePath items.
You also have other items as inputs too. Should we include any of them as well?
There was a problem hiding this comment.
Probably a good idea. We should also get a review from the compiler team to make sure what we end up going with makes sense.
There was a problem hiding this comment.
Hard to comment here cause I'm unsure exactly what this part of MSBuild is achieving. Hence can't say what is and is not good items to be using here.
There was a problem hiding this comment.
As I understand it, the FUTD check looks at the CoreCompileCache to see what items need to be up-to-date for it to be able to skip executing CoreCompile. Currently CoreCompileCache is missing things that are actually used by CoreCompile, so if you update EmbeddedResources, the FUTD check says it's up-to-date and skips CoreCompile anyway. This aims to fix that, but we'd also like to hit other things that, when changed, should make CoreCompile run again that are currently missed.
There was a problem hiding this comment.
This does not have anything to do with FUTD, which is a project system concept.
This target tries to determine whether the set of inputs to CoreCompile has changed since the last time the compiler was run. It was added because of a scenario new to globbing support:
- Build project.
- Delete source file
foo.cs. - Build again
CoreCompilethinks it's up to date because the output.dllis newer than all inputs, but that's wrong because an input was removed.
Prior to extensive use of globbing, the $(MSBuildAllProjects) input to CoreCompile caught this case because the project file was modified to remove the source file. But with globbing, we needed a build-time step.
This PR is extending this from @(Compile) items to include @(EmbeddedResource) which has a similar problem--we could skip running the compiler and leave an output with an extra embedded resource.
The question is: are there other inputs likely derived from globs in the SDK case that we should consider here?
There was a problem hiding this comment.
That's my question as well… #5334 (comment)
From what I see, we have modules, embedded files, strong name key and Win32 manifest, Icon and resources!
Not to mention WPF items and its 2-pass compilation! That's on a whole 'nother level!!
There was a problem hiding this comment.
The question is: are there other inputs likely derived from globs in the SDK case that we should consider here?
What all can globs be used for now? Should my mental model just be anything on disk? If so then probably need $(AdditionalFiles), references and $(EmbeddedFiles) at least.
I omitted $(EditorConfigFiles) because I think those are handled specially in MSBuild already.
There was a problem hiding this comment.
Yep, @(AdditionalFiles) and @(EmbeddedFiles) probably need to be checked too: Additional files can affect generators and analyzer warnings, and the embedded files are put directly into the assembly (different but similar to EmbeddedResources).
Regarding @(EditorConfigFiles) while the collection is created by MSBuild logic rather than user globs, I suspect it still has the same problem. If you delete an editorconfig file, then although the collection will change, CSC is still going to see the DLL as being more up to date than any of the inputs, which can again affect analyzer warnings etc.
There was a problem hiding this comment.
Unfortunately I was pretty misleading above. Sorry! Despite what I said, this isn't just about globs. It's really any multi-valued thing that can be influenced via something other than editing the project file. That's why $(DefineConstants) is there below: if you build, then build with msbuild -p:EnableFancyDebugMode=true, you'll be surprised if we don't run the compiler with the new defines.
Unfortunately, this means that the set of things we'd ideally define here is essentially "all inputs to Csc/Vbc, which isn't maintainable cross-repo.
We'll have to try to find a reasonable middle ground. The ones you suggest sound pretty good to me, since we already have defines.
I agree on EditorConfigFiles -- IIRC the special logic is about discovering them, rather than accounting for them here; I bet the same "delete a subfolder's .editorconfig and watch it not recompile" thing would apply.
|
@Nirmal4G would it be possible for you to ping me by email as I had something to chat with you about? My email address is marcpop@myemployer Thanks. |
|
@marcpopMSFT I have sent an e-mail as you requested. |
|
So, We'll add What about ASP.NET/WPF/UWP/WinForms includes? Sure, we are not going to add vendor specific here but do they need to be added too (in their own build targets)? |
Caching input metadata for `CoreCompile` target is achieved by adding the items to the `CoreCompileCache` item in the `_GenerateCompileDependencyCache` target. To cache the `EmbeddedResource` items, we just add those to the existing inputs through `CoreCompileCache` item, to include them in the cache file which gets included in the `CoreCompile` target.
Nirmal4G
requested review from
jaredpar,
rainersigwald,
Forgind and
chsienki
|
It seems I can't add any more commits to this and to many of my older PRs. I suspect because of the reforking due to dependabot issues (that is still not yet resolved). So, at least for the issue mentioned, this patch fixes it. If we need tests to cover this behavior, I could open a new PR with all we talked about or else we merge this now and I open a new PR with remaining changes and tests. Let me know! |
What dependabot issues are you referring to here? |
|
It created PRs in my fork! I couldn't at first disable them and then docs asked me to re-fork the Repo and disable them manually. It did the trick (I think) but it resulted in repo deletion and thus removing all the PR refs. But forking the repo again didn't restore the PR refs (GitHub told me the behavior is intended and to stop injection attacks). |
|
@Nirmal4G do you plan to continue work on this PR or should that be closed? (a new one can allways be open - which might as well be quickes solution to your GH issues) |
|
Hey @Nirmal4G, kindly pinging :) Can we close the PR, or do you plan to continue? |
Fixes #5334
Context
Fast up-to-date check with SDK project doesn't see new embedded resources!
Changes Made
To cache the
EmbeddedResourceitems, we just add those to the existing inputsthrough
CoreCompileCacheitem, to include them in the cache file which getsincluded in the
CoreCompiletarget.Testing
Manually verified using MSBuild Log Viewer with the repro present hyrmn/ReproGlobsNotEmbedding.
Notes
See dotnet/project-system#5794 for more details.