Msbuild: DefineConstants should be considered for CoreCompile incrementality

[rainersigwald]

_From @JustArchi on May 25, 2018 19:58_

Let's assume that you have a C# project similar to below:

#if CUSTOM_PUBLISH
public static readonly CustomPublish = true;
#else
public static readonly CustomPublish = false;
#endif

With proper DefineConstants in csproj:

  <PropertyGroup Condition="'$(CUSTOM_PUBLISH)' != ''">
    <DefineConstants>$(DefineConstants);CUSTOM_PUBLISH</DefineConstants>
  </PropertyGroup>

If you execute following two commands:

dotnet build
dotnet publish -o out /p:CUSTOM_PUBLISH=true

Then execute out\Program.dll, CustomPublish will still be false as dotnet publish did not trigger project rebuild done previously by dotnet build which didn't use the switch. This will not happen if you execute dotnet publish command on the clean tree, without executing dotnet build firstly.

My first question, is this intended? If yes, what is the recommended way to go about this? dotnet clean, or maybe something else? I can understand the rationale behind that, but I'm not entirely sure if dotnet should act in this way.

Thank you in advance.

_Copied from original issue: dotnet/cli#9349_

[rainersigwald]

_From @livarcocc on May 25, 2018 21:17_

I don't think this is an issue in the SDK. I tried on a non-sdk style project and I got the same results. This may be MSBuild's incremental check.

@rainersigwald are you aware of any such issues on MSBuild's side?

[rainersigwald]

This is a general problem with the MSBuild incrementality model, but in this case we have already developed a mechanism to solve it. I'll move this to MSBuild and elaborate there.

[JustArchi]

Thanks for moving. I'll keep looking here 🙂

[rainersigwald]

The core problem here is that MSBuild incrementality depends on the output file being newer than its input files. That's still the case here, because no files have changed--but _the output of the compiler_ would change if it were to run again.

Fortunately, we have a way to deal with cases like this:

https://github.com/Microsoft/msbuild/blob/e7ea68da3ee0929a07cc36e877a32d5e444efefb/src/Tasks/Microsoft.Common.CurrentVersion.targets#L3404-L3431

We should add $(DefineConstants) to that hash, so that it causes the compiler to rerun if it doesn't match the last run.

This isn't a fully general solution to the missed-builds-on-property-changes problem. That's a much bigger issue, considered in #701.

[JustArchi]

Is there any way to patch this in csproj? I'm wondering if there is some possible workaround to use until this issue is dealt with.

[dasMulli]

@JustArchi yes you can add this (not the universally correct approach but should be enough for your case):

  <ItemGroup>
    <CoreCompileCache Include="$(DefineConstants)" />
  </ItemGroup>

[JustArchi]

This worked for me, thank you a lot! 🎉

[dasMulli]

@rainersigwald any objections to considering $(DefineConstants) by default? Seems quite reasonable.. or should this somehow be done by roslyn targets? (after all, it is their concern)

[rainersigwald]

I think we should do it, and 16.0 is a great time for the change. But we should run it by the compiler folks.

@jaredpar would you object to us adding $(DefineConstants) to @(CoreCompileCache) so that CoreCompile is rerun if constants change (as well as source-list changes)?

[jaredpar]

That seems pretty reasonable to me. Well more it just seems correct to do this.

CC @agocke in case I'm missing anything.