Project-system: UpToDateCheck does not work with CLI tools

[Pzixel]

Visual Studio Version: 16.3.5

Summary: I found an article where they show how to use this new setting. The problem is that this setting restart build but only partially. This way build artifacts differs from when I use full rebuild.

Steps to Reproduce:

1. git clone -b UpToDateCheckBug --single-branch https://github.com/Pzixel/Solidity.Roslyn.git
2. cd Solidity.Roslyn
3. dotnet run --project Solidity.SampleApp/Solidity.SampleApp.csproj
4. touch Solidity.Roslyn.Example/Sample.sol
5. dotnet run --project Solidity.SampleApp/Solidity.SampleApp.csproj

Expected Behavior:
Buld timestamp will be different since rebuild caused CLI tool to regenerate BuildInfo class.

Actual Behavior:
File don't get regenerated and we get the old time stamp.

User Impact:
Users are forced to rebuild the whole solution all the time because they can't be sure if their code change should cause regeneration.

[drewnoakes]

Thanks for filing a separate issue 👍

The documentation you linked to about the _fast_ up-to-date check, which is a component of Visual Studio and isn't involved in your CLI repro above.

In your branch I see this edit:

  <ItemGroup>
-    <CustomAdditionalCompileInputs Include="**\*.sol" />
+      <UpToDateCheckInput Include="**\*.sol" />
  </ItemGroup>

Is there a reason for this change? Was your up-to-date check not working in VS before this change? If you revert the above diff does everything work correctly in both VS and on the command line?

I'll look to see if we can improve the documentation to make this clearer.

[Pzixel]

Is there a reason for this change? Was your up-to-date check not working in VS before this change? If you revert the above diff does everything work correctly in both VS and on the command line?

Yes, there is a reason: it didn't work with CustomAdditionalCompileInputs. I'm actually looking for any way to make my flow working with regular builds but I'm failing so far.

The documentation you linked to about the fast up-to-date check, which is a component of Visual Studio and isn't involved in your CLI repro above.

The VS behavior via 'build/rebuild' buttons is the same.

[drewnoakes]

Can you explain more about why you changed it? Was it previously working on CLI/VS/neither? Does it now work on CLI/VS/neither?

[Pzixel]

I didn't work in neither VS or CLI. You can see it yourself. Open project in VS, rebuild, see build time, then change anything in sol file and build it again. You won't see any changes in output. In a nutshell: neither UpToDateCheckInput or CustomAdditionalCompileInputs don't work in neither VS or CLI.

[davkean]

@drewnoakes Please investigate and figure out what the cause is.

[drewnoakes]

@Pzixel I took a look at your solution and cannot reproduce the behaviour you're seeing. It would help if you could be more specific about what you see and what you expect.

From my investigation I do not believe this to be a bug in VS or MSBuild. Your issue may be related to the code generation package you're using, potentially as the package's targets don't specify inputs/outputs and so MSBuild does not build incrementally:

https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-build-incrementally?view=vs-2019#specify-inputs-and-outputs

It's possible that the target(s) in this file should be specifying their inputs and outputs:

https://github.com/AArnott/CodeGeneration.Roslyn/blob/master/src/CodeGeneration.Roslyn.BuildTime/build/CodeGeneration.Roslyn.BuildTime.targets

Please file an issue in that package's repository. If you link it here I will subscribe to it to follow its progress. Thanks.

[Pzixel]

@drewnoakes hmm, you didn't see the behaviour I described above? That sounds really strange because this is why I created this MRE branch. Is have to show the problem.

It's possible that the target(s) in this file should be specifying their inputs and outputs:

I thought this is VS related issue because I have seen "dotnet cli call" in logs but it didn't produce the required result so I expected VS calls some "fast path" which isn't valid in all cases.

I will create an issue in the repo package I'm using but I don't think their inputs should be related. I have a file and I say "rebuild every time this file changes". It doesn't matter if any other tool is looking at it, you just have to rebuild when it's changed. It works when I manually click "rebuild" but it doesn't when we consider this project option. This is why I suspected VS to be a culprit.

[drewnoakes]

@Pzixel I took another look at this and have a few observations.

Firstly, I think you need to add your .sol file as both UpToDateCheckInput (for the VS fast up-to-date check) and as CustomAdditionalCompileInputs for the MSBuild up-to-date check. There was a time when VS considered CustomAdditionalCompileInputs too, but IIRC @rainersigwald advised against this. Rainer does it seem right that people should have to specify custom inputs in multiple item groups?

Secondly, it seems that the generator package is not working as you might expect. The BuildInfo type is only regenerated if the Solidty.Roslyn project is rebuilt.

If you instead run:

dotnet run --project Solidity.SampleApp/Solidity.SampleApp.csproj
touch Solidity.Roslyn/SolidityGenerator.cs
dotnet run --project Solidity.SampleApp/Solidity.SampleApp.csproj

...you will see different timestamps on the second invocation.

Perhaps you can put some console debugging in your generator function and pass --verbosity normal to your dotnet run command.

I don't believe this has to do with the up to date check.

[Pzixel]

But touching SolidityGenerator.cs doesn't make any sense. Of course you can touch any cs file in Solidity.Roslyn directory and it will make the whole thing rebuild but it's just because standard dotnet toolchain sees that file has changed to it rebuilds the package. And transitively rebuild all the depended packages.

That makes sense. But if you consider Solditiy.Roslyn to be shipped as a nuget package then you don't have a file to tough because you have no project, just a dll with some roslyn generator. I only have shown it for simiplicity reasons but in reality you have just one sol file and dll that works with it.

I think the project I use for generation could provide some additional info to explain why the full rebuild is required but I think the fact that file listed in UpToDateCheckInput/CustomAdditionalCompileInputs has changed is enough to rebuild the project.

---

In a nutshell, there should be no difference between manual "rebuild project" button hit and "build project" button hit if file listed in UpToDateCheckInput has changed. Now there is clearly a difference, no matter if other tools knows anything about VS processes. If they work correctly when you rebuild the project they must work correctly when they just build. And I think it's up to VS to say "go full rebuild guys".

[drewnoakes]

My testing shows that the project _is_ rebuilding, but the generated code is not being changed unless I touch SolidityGenerator.cs. I agree that makes no sense for what you're wanting, but at this point I think your question is best suited for the library author as I don't see it having anything to do with either the VS fast-up-to-date check, or MSBuild's up-to-date check.

[Pzixel]

In my opinion trigger for manual rebuild and rebuild_if_anything_has_changed should be the same.

In this case there won't be any difference in outputs since it's the same.

However currently it clearly works in different ways. I think we can cooperate with library author to see what differs but it's somewhat beside the point.

I mean triggering build this or other way should be indistinguishable interchangeable. External tool shouldn't concider different build strategies, it's, well, a build. This is the only reason why I'm insisting on fixing it on project-system side: tool shouldn't know the difference between manual rebuild and rebuild because of check. If it doesn't know it it will produce the same output for sure.

Otherwise I'd ask library authors to fix it on their side. But this problem is not specific for the lib. Other libraries may act on build in different ways and they could face the same problem. The problem that manual build doesn't trigger the same process as build by check.

In a nutshell I think that the difference between two build kinds is internal and shouldn't be exposed to the tools, or it should be at least the same by default.

[rainersigwald]

Firstly, I think you need to add your .sol file as both UpToDateCheckInput (for the VS fast up-to-date check) and as CustomAdditionalCompileInputs for the MSBuild up-to-date check. There was a time when VS considered CustomAdditionalCompileInputs too, but IIRC @rainersigwald Rainer Sigwald FTE advised against this. Rainer does it seem right that people should have to specify custom inputs in multiple item groups?

I think this was answered by further discussion, but: No, I don't think this makes sense, but I don't think it should be required. In the everything-is-behaving-correctly case:

• The true inputs are known for the VS fast up-to-date check (via UpToDateCheckInput).
• The code generator has correct incremental behavior and adds a generated source file to the normal compile inputs
• The compiler then runs when the .sol file changed, because that resulted in an updated .g.cs file. But the compiler's inputs don't need to care about the .sol.

CodeGeneration.Roslyn is not updating the .generated.cs file in your repo because its internal incrementality check

https://github.com/AArnott/CodeGeneration.Roslyn/blob/246ca6a3a01b24f2793293f03ed80e99a7ba0ed1/src/CodeGeneration.Roslyn.Engine/CompilationGenerator.cs#L135-L138

considers only the input syntax tree (in this case Solidity.cs), not any additional inputs your particular generator may read from. I think that's reasonably tracked by AArnott/CodeGeneration.Roslyn#127.

[Pzixel]

@rainersigwald right. It seems that I'm wrong and my tool explicitly perform its own check to skip builds. Sorry for bothering you guys. Awesome work.

[drewnoakes]

@rainersigwald thanks for your insight and for joining the dots to that existing issue. I missed that.

@Pzixel no bother at all. This was interesting and I learned a few things along the way. Good luck getting this resolved.