dotnet / project-system

The .NET Project System for Visual Studio
MIT License
968 stars 387 forks source link

UpToDateCheck: always fail if projects.assets.json is updated #6227

Closed xen2 closed 2 years ago

xen2 commented 4 years ago

Visual Studio Version: 16.5.5

Summary:

Steps to Reproduce:

  1. Create .NET Standard project

  2. Build twice (UpToDateCheck should work on second build)

  3. Touch obj\projects.assets.json (in practice, it happens to me once in a while when VS restore solution)

  4. Build once: UpToDateCheck fails (expected) and trigger build However output files are not being touched because CoreCompile is skipped (Inputs don't contain obj\projects.assets.json, only .cs files)

Expected Behavior:

  1. Build again: UpToDateCheck succeed

Actual Behavior:

  1. Build again: UpToDateCheck fails (because output files are still older than obj\projects.assets.json)

User Impact:

UpToDateCheck is broken until a full Rebuild

Workaround

Here's a MSBuild Target to add this file to CoreCompile inputs:

  <Target Name="_GenerateCompileInputsProjectAssets" AfterTargets="_GenerateCompileInputs">
    <ItemGroup>
      <CustomAdditionalCompileInputs Include="$(ProjectAssetsFile)" />
    </ItemGroup>
  </Target>
ocallesp commented 4 years ago

@rainersigwald I think this is similar to the issue when we copy&paste files (keep the old timestamp) to a project. CoreCompile will not take into consideration those copied/touched files.

Input 'C:\Users\oscalles\source\repos\ConsoleApp31\ConsoleApp31\obj\project.assets.json' is newer (5/23/2020 8:39:34 PM) than earliest output 'C:\Users\oscalles\source\repos\ConsoleApp31\ConsoleApp31\bin\Debug\net5.0\ConsoleApp31.pdb' (5/23/2020 8:37:30 PM), not up to date. (ConsoleApp31)

The project is compiled because FUTD detect is out of date. After the build, the *.pdb doesn't change the timestamp

xen2 commented 4 years ago

@ocalles I can confirm this is likely the same issue (I got a similar log).

rainersigwald commented 4 years ago

Yes, it's related. Unlike the other problem, though, I don't think there's a clean answer to this. That file is a project-level input; it can result in changes that require recomputation (for instance, changing version of a referenced package can pull in a different DLL). But it doesn't necessarily do that; just touching the file for instance would trigger a build from FUTD but it's completely correct for MSBuild to skip the compiler: none of its inputs have changed.

So this is a case where the project system's more expansive understanding of "inputs" clashes with the ground truth. Do we know why project.assets.json is considered an input? It's an output of NuGet restore so I would expect the project system to ignore it (in favor of actual inputs like project files, NuGet.config, etc).

ocallesp commented 4 years ago

My understanding is that project.assets.json is not required for builds, BUT if it exist FUTD will take it as an input item because the assumption is that changes to project.assets.json might affect the build output.

drewnoakes commented 4 years ago

I'm not certain that project.assets.json should be an input to the fast-up-to-date check.

It's coming in via AdditionalDependentFileTimes which we use for other files which can influence compilation such as .editorconfig, global.json, Directory.Build.*.

@davkean can you think of a reason that the assets file should be an input?

davkean commented 4 years ago

Was the reason you checked AdditionalDependentFileTimes literally to cover the assets file?

davkean commented 4 years ago

https://github.com/dotnet/project-system/pull/6013

drewnoakes commented 4 years ago

6013 also handles removal of .editorconfig or global.json files.

Can we:

davkean commented 4 years ago

Don't use ProjectAssetsFile; restore can be turned off and those projects will never be considered up-to-date. Use its presence inside AdditionalDependentFileTimes as that indicator.

ocallesp commented 4 years ago

In #6013 we check the timestamp when the file is added or removed. We don't check for changes to project.assets.json. If the file existed, but suddenly removed the project is out of date.

In this issue is about the timestamp when the file changed. This project.assets.json in AdditionalDependentFileTimes is not required, but we still compare the timestamp.

davkean commented 4 years ago

Drew and I have sync'd offline, a couple of things:

Project.asssets.json -> Drew, we didn't discuss this, but I'm thinking that Rainer might be right on this, on its deletion we should be trigging an evaluation -> design-time build which should be changing the other inputs (ie removal of references, in particular) that we're tracking. Why isn't this the case? Does CPS not trigger the build because the file disappeared?

Potential Global.json locations and Potential EditorConfigs locations all have the same properties as the original reason that MSBuildAllProjects was added; adding, removing or changing them has the potentional ability to affect the output (change targets files, introduce errors, or in case of source generators, affect the output), but we don't know that upfront. If a targets file changes, Csc always builds, perhaps the same thing should happen for these? If I change global.json today to point to a different SDK, does MSBuild rebuild?

RobSwDev commented 4 years ago

I also have this issue. I and it makes our large (150 project) solution build much more slowly - even though when the build drops into MSBuild, MSBuild realises there's no need to call the compiler. Disabling package restore during build helps in theory, otherwise projects.assets.json gets touched every build. But I don't know how to prevent it getting touched when the solution is opened.

The work-around for me is, after opening the solution, wait for the automated background package restore to complete (I swear this restore sometimes goes into a loop) and then do a (in theory unnecessary) full rebuild, to ensure that the dlls and pdbs are all newer than any assets.json file. I get correct FUTD behaviour after that, but it's a bit of a pain.

panopticoncentral commented 4 years ago

I would add a comment here that is orthogonal to the bug but which relates to some of the comments like the one from @RobSwDev. NuGet should only touch the assets file on restore if something actually changed (i.e. if it actually did a restore). If you're seeing the assets file getting touched even when everything was restored already, that's a NuGet bug and should be reported to them. In addition to causing up-to-date checks to fail, asset file changes cause design-time builds to happen which can slow down VS after solution open.

nkolev92 commented 4 years ago

I'm investigating a potentially related problem, @xen2 did this start reproing with 16.5? Do you remember if it was a problem in 16.4?

xen2 commented 4 years ago

@nkolev92 sorry that I didn't answer at the time. I don't remember exactly when the problem started.

drewnoakes commented 2 years ago

@xen2 do you still see this issue?

drewnoakes commented 2 years ago

I just tried the repro steps here and it no longer reproduces in 17.1. We have made several changes to the fast up-to-date check since this issue was filed, and looking at the code I do not expect this issue to exist any more. I expect the fix exists in 16.11 (possibly earlier).

I'll close this. Let me know if you still experience this issue. Thanks.