Make GeneratePublishDependencyFile incremental

[sbomer]

Fixes #28661 by making GeneratePublishDependencyFile incremental. The incremental inputs are the same as those used for GenerateBuildDependencyFile.

[dsplaisted]

The It_compresses_single_file_as_directed test is failing, it looks like because the EnableCompressionInSingleFile changes something which is not accounted for in the inputs and outputs that are declared for the GeneratePublishDependencyFile target.

Probably every property / file that is read in the target should be part of the inputs. However, you can't declare properties as inputs to a target since they don't have a corresponding timestamp to let you know if they changed. What we do in this type of situation is to create a separate target which hashes all of the inputs and writes them to a file with WriteOnlyWhenDifferent="True", so the timestamp only gets updated when the properties change. You can see an example of this with the _GenerateRuntimeConfigurationFilesInputCache target.

[sbomer]

Thanks for the pointers @dsplaisted.

@agocke suggested not adding another instance of the property caching fix, and instead increasing the refcount for the underlying issue (I think dotnet/msbuild#701 is tracking it). There are other instances of this problem in publish, for example dotnet/linker#2102.

So I updated the testcase to avoid relying on incremental publish behavior instead (what it's testing is orthogonal to incremental build issues).

[sbomer]

@dsplaisted PTAL

[dsplaisted]

I think this change as is would be a regression. Previously if you changed the value of EnableCompressionInSingleFile on the command line, it would always regenerate the deps file. Now if you only change that value it won't regenerate it and the output will be wrong.

There are probably other properties that would be similarly broken, this was just the one that we happened to have test coverage for.

It sounds like there are also other properties that are already broken in this way. However I wouldn't want to introduce new broken behavior without having a way to be sure that the impact would be minimal.

We should discuss (probably in the MSBuild / CLI partner sync) whether we should fix the underlying issue. If not, then we may need to add a bunch of explicit input hashing targets to fix the incremental behavior with command-line specified global properties.

[sbomer]

I agree with everything you've said - I was just a little more willing to take the risk of new broken behavior, given that this seems to already be a widespread problem. But I'm willing to implement the caching fix if needed to fix this issue.

Is there something I can do to put this up for discussion at the MSBuild/CLI next partner sync? Happy to attend if that would be useful.

[agocke]

We should discuss (probably in the MSBuild / CLI partner sync) whether we should fix the underlying issue

In particular, I think we need a broad design solution to this problem. Given that this is pervasive in the system, the right message for users, right now, is that you should not expect incrementality of properties.

[sbomer]

Based on the discussion, I think we are considering property-based incrementality as not supported until we do a broader effort to fix this across the SDK. Does this fix look ok as-is @dsplaisted?

[dsplaisted]

A target can be:

• Not incremental - it always runs for each build that requires it, whether inputs have changed or not
• Correctly incremental - it only runs if inputs have changed
• Incorrectly incremental - Sometimes it doesn't run even if inputs have changed

There are targets which are already incorrectly incremental, and I think it's OK to not fix them until we have better MSBuild incrementality support. But I don't think we should change targets from "not incremental" to "incorrectly incremental". If we want to add incrementality to the target, I think we should try to make it correct.

So for this PR, if we do want to make GeneratePublishDependencyFile incremental, I think we should do the hashing to make it correct.

[agocke]

@dsplaisted Makes sense. Is there a public writeup of what we need to do here for best practice?

[dsplaisted]

@dsplaisted Makes sense. Is there a public writeup of what we need to do here for best practice?

Not yet. @rainersigwald has an action item to write that up

[sbomer]

@dsplaisted I added the property hashing fix to the two targets where it was required for this testcase. PTAL.

[dsplaisted]

It may be better to include FilesToBundle in the hash rather than as a normal input so that if a file to bundle is removed, it is handled correctly.

[sbomer]

I added this to the hash, but I think it also needs to be a normal input - that way it'll rebuild if the list of filenames changes, or if the file timestamps changes.

[dsplaisted]

A lot of the item inputs aren't declared here or included in the hash. Is this because MSBuildAllProjects will pick up any changes to the project file, and any properties passed on the command line will probably change properties that are included in the hash?

[sbomer]

I didn't include the items in Inputs or the hash because they weren't included in the incremental inputs for GenerateBuildDependencyFile either, and I would suspect that they are only likely to change when the project file changes.

I did try to include all relevant properties in the input hash, except for $(MSBuildProjectFullPath), since I thought that would likely invalidate most of the build already - but I'll add it to be safe.