-
Notifications
You must be signed in to change notification settings - Fork 107
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.
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
Remove inputs which are not files from CsWinRTGenerateProjection target. #1526
Conversation
Going to trigger a build and see what happens in local incremental scenarios. |
I don't have permission to leave a review but I approve this change. 😁 |
I don't think we should remove the inputs, but probably need to specify differently how they are consumed as inputs, for example by hashing them and writing to a file and then use the file as input. This is how CoreGenerateAssemblyInfo in the .NET SDK does this. |
@jlaanstra is it not sufficient that the inputs ultimately came from a csproj/props/targets? Or can they flow in from computed results too? |
If you change those properties, you must have changed your csproj and those will cause a rebuild of your project... right? |
Nothing prevents these properties to be defined dynamically without requiring changes to csproj/props/targets. |
@Jeremy-Price do you want to take a stab at the CoreGenerateAssemblyInfo approach? Like this https://github.com/dotnet/sdk/blob/ba45654d71b1da7cc139285b312e2f152febc008/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets#L171 |
Sure, I'll take a look at it. I am not an msbuild expert though so I may need some help if I get lost. |
Speaking of not being an msbuild expert--is there no way to insist that a property value comes from a source file? Or detect if it doesn't? Or put the values into a real file if they aren't backed by one already, and use that as an input? If anything can change like this, it seems that every build target is potentially in trouble if it doesn't do something similar. |
One other thought I had that is similar to the hashing approach is, we instead generate the |
This approach is pretty uncommon. Most targets don't do this. Who is dynamically changing these inputs that the ROI of doing this work is worth it? |
Is it possible for us to just document (or enforce?) that these properties must only be provided in csproj/props/targets files? Would changing these dynamically even work with all the other parts of the build process? |
We currently use cswinrt.rsp as the output marker so we'd need to find a different output marker. |
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.
Totally missed the use of MSBuildAllProjects. That would indeed probably make this work reasonably well.
I can see why someone might include "dynamic" stuff in the generated AssemblyInfo.cs (time of build, git commit hash, etc), making "perfect dependency tracking" a necessity in CoreGenerateAssemblyInfo. However, going for perfect dependency tracking is the exception, not the norm in MSBuild targets. For example, if I define CheckForOverflowUnderflow based on the time of day, that'll break my incremental builds. A good parallel for the dependency of CsWinRTGenerateProjection on the value of CsWinRTExcludes, would be the dependency of CoreCompile on the value of CheckForOverflowUnderflow. It's also an imperfect dependency (it doesn't work if I define CheckForOverflowUnderflow based on the time of day, or override it on the command line), but it's considered good enough, that's ensured by mainly two things:
So I think as long as CsWinRTGeneratedFilesDir includes Configuration, dependency tracking here is as good as in CoreCompile. |
Did some basic testing of a couple incremental scenarios. Things seem to be working as expected and MSBuildAllProjects is indeed being detected as changed when you change like a csproj to adjust the namespaces to project. |
The msbuild documentation states that the
Inputs
property of a target must be a list of files. However,$(CsWinRTExcludes);$(CsWinRTIncludes);$(CsWinRTExcludesPrivate);$(CsWinRTIncludesPrivate);$(CsWinRTFilters);$(CsWinRTWindowsMetadata)
are not files, so the target does not run incrementally as the non-files are always absent.I think it's just safe to just remove these, if they are changed then a project file or targets file will have changed, which should trigger the correct incremental build behavior.