dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.17k stars 1.34k forks source link

MSBuild-task equivalent of "msbuild.exe /restore /t:Target" #2811

Open natemcmaster opened 6 years ago

natemcmaster commented 6 years ago

https://github.com/Microsoft/msbuild/pull/2414 added support for msbuild /restore /t:Target which executes /t:Restore in a new build context before invoking /t:Target.

Is it possible to perform the same behavior from the MSBuild task?

For example, i'd love to be able to do this

<Project>
  <Target Name="CiBuild">
      <MSBuild Projects="MyProj.sln"
               Targets="Build"
               Restore="true" />
   </Target>
</Project>

cc @jeffkl

jeffkl commented 6 years ago

At this time, it is not possible to do this. The logic for /restore is pretty proprietary to MSBuild.exe while the <MSBuild /> task is using a completely different code path. That said, anything is possible, however we do not currently have plans to implement this.

natemcmaster commented 6 years ago

That's what I feared, after taking a cursory glance at #2414.

AndyGerlicher commented 6 years ago

Can you just turn that into two MSBuild task invocations and set different global properties? Targets="Restore" Properties="IsRestoring=true" then call Build.

natemcmaster commented 6 years ago

That's what we've done in the past to force project re-evaluation after restore. The main problem with that is mostly usability. It's not immediately obvious to devs new writing MSBuild--or even experienced ones--why you need separate MSBuild-task invocations, and why you need a "dummy" property on the restore one.

dasMulli commented 6 years ago

For CI projects, I've been moving them to having an explicit Restore target in them so I can call them through msbuild /restore ci-build.proj or just dotnet build ci-build.proj. https://gist.github.com/dasMulli/fdc9bf5c433175f8feb638d3eed41b68

nguerrera commented 6 years ago

The main problem with that is mostly usability. It's not immediately obvious to devs new writing MSBuild--or even experienced ones--why you need separate MSBuild-task invocations, and why you need a "dummy" property on the restore one.

+100 and TIL that it's not correct either. I'll let @rainersigwald explain why. ;)

rainersigwald commented 6 years ago

The reason it's not sufficient to use a global property is that MSBuild maintains a ProjectRootElementCache of the parsed XML of imported files. It's there for performance (it would be slow and unnecessary to load Microsoft.Common.CurrentVersion.targets for every project in your build) but also for consistency, ensuring that if you edit a common import mid-build you don't have half of your projects with the old version and half with the new version.

Self-contained demo of the problem:

<Project DefaultTargets="Build">
 <PropertyGroup>
  <Import>import.props</Import>
 </PropertyGroup>

 <Import Project="$(Import)" Condition="Exists($(Import))" />

 <PropertyGroup>
  <ReadValue Condition="'$(ReadValue)' == ''">0</ReadValue>
 </PropertyGroup>

 <Target Name="Restore">
  <PropertyGroup>
   <WriteValue>$([MSBuild]::Add($(ReadValue), 1))</WriteValue>
 </PropertyGroup>
  <ItemGroup>
   <IncludeLines Include="&lt;Project&gt;" />
   <IncludeLines Include=" &lt;PropertyGroup&gt;" />
   <IncludeLines Include="  &lt;ReadValue&gt;$(WriteValue)&lt;/ReadValue&gt;" />
   <IncludeLines Include=" &lt;/PropertyGroup&gt;" />
   <IncludeLines Include="&lt;/Project&gt;" />
  </ItemGroup>

  <WriteLinesToFile File="$(Import)"
                    Lines="@(IncludeLines)"
                    Overwrite="true" />

  <Message Importance="High" Text="Wrote value: $(WriteValue)" />
 </Target>

 <Target Name="Build">
  <Message Importance="High" Text="Read value: $(ReadValue)" />
 </Target>

 <Target Name="BuildWithDifferentGlobalProperties">
  <MSBuild Projects="$(MSBuildThisFileFullPath)"
           Targets="Restore"
           Properties="_DummyProperty=Restore" />
  <MSBuild Projects="$(MSBuildThisFileFullPath)"
           Targets="Build"
           Properties="_DummyProperty=Build" />
 </Target>
</Project>
s:\repro\Microsoft\msbuild\issues\2811>msbuild /nologo /v:m demo-global-props-insufficient.proj /t:Build;Restore
  Read value: 0
  Wrote value: 1

s:\repro\Microsoft\msbuild\issues\2811>msbuild /nologo /v:m demo-global-props-insufficient.proj /t:Build;Restore
  Read value: 1
  Wrote value: 2

s:\repro\Microsoft\msbuild\issues\2811>msbuild /nologo /v:m demo-global-props-insufficient.proj /t:BuildWithDifferentGlobalProperties
  Wrote value: 3
  Read value: 2

s:\repro\Microsoft\msbuild\issues\2811>msbuild /nologo /v:m demo-global-props-insufficient.proj /t:BuildWithDifferentGlobalProperties
  Wrote value: 4
  Read value: 3

s:\repro\Microsoft\msbuild\issues\2811>msbuild /nologo /v:m demo-global-props-insufficient.proj /restore /t:Build
  Wrote value: 5
  Read value: 5

s:\repro\Microsoft\msbuild\issues\2811>msbuild /nologo /v:m demo-global-props-insufficient.proj /restore /t:Build
  Wrote value: 6
  Read value: 6

I'm not totally sure I buy the consistency argument for requiring the cache within a build, but I also don't know how to validate that relaxing it won't break some VS customer somewhere.

rainersigwald commented 6 years ago

We had a bug for that I just ran across: https://github.com/Microsoft/msbuild/issues/1185, including a case where the "snapshot" behavior is required for correctness.

msshapira commented 5 years ago

Any update on this issue? Is there a good workaround?

PhilipDaniels commented 5 years ago

Just wanted to say that I have wasted 2 days of time because /t:restore;build does not work reliably, before coming across this amazing comment https://github.com/Microsoft/msbuild/issues/3000#issuecomment-417675215 Judging by queries on Stack Overflow I am not the only one. Incidentally, I have not seen this documented anywhere in the official docs.

The suggested workaround of using /restore did not work either, I had to add a specific old-style build step of calling nuget.exe to do the restore. This was on a Bamboo CI server.

If /t:restore and /restore cannot be made to work properly then you should remove them or print out a message recommending alternatives and then fail the build. At least that way people won't waste their time.

aolszowka commented 5 years ago

@PhilipDaniels the problem is /t:restore DOES work properly; however it must be the only task called during that process; /t:restore;build DOES NOT work.

That is why we have several tasks that look like this:

  <Target Name="TIMSNETNuGetRestore" DependsOnTargets="GetTIMSNETSolution">
    <Message Text="Restoring NuGet Packages for TIMSNET" Importance="high"/>

    <!-- You're going to be super temped to combine this with BuildTIMSNET -->
    <!-- However you cannot because of bugs in the context; see the common -->
    <!-- "Restore" target in ComputersUnlimited.Build.All.msbuild          -->
    <MSBuild
      Projects="@(ProjectsToBuild)"
      Properties="PostBuildEvent="
      Targets="Restore"
      BuildInParallel="true" />
  </Target>

And its caller

  <!--**********************************************************************-->
  <!--* Restore (DO NOT CHANGE THIS NAME)                                  *-->
  <!--*   This is the target that will be executed in its own context when *-->
  <!--* you do an "msbuild /restore" and this is where all of the NuGet    *-->
  <!--* packages should be restored.                                       *-->
  <!--*                                                                    *-->
  <!--* See the Following:                                                 *-->
  <!--*    - https://github.com/Microsoft/msbuild/issues/2811              *-->
  <!--*    - https://github.com/Microsoft/msbuild/issues/3000              *-->
  <!--*      Specifically the @aolszowka comments                          *-->
  <!--**********************************************************************-->
  <Target Name="Restore">
    <CallTarget Targets="TIMSNETNuGetRestore" />
  </Target>
rainersigwald commented 5 years ago

The suggested workaround of using /restore did not work either, I had to add a specific old-style build step of calling nuget.exe to do the restore. This was on a Bamboo CI server.

@PhilipDaniels Can you please open a new issue describing what you did and what went wrong? That's expected to work.

PhilipDaniels commented 5 years ago

The suggested workaround of using /restore did not work either, I had to add a specific old-style build step of calling nuget.exe to do the restore. This was on a Bamboo CI server.

@PhilipDaniels Can you please open a new issue describing what you did and what went wrong? That's expected to work.

@rainersigwald Done https://github.com/Microsoft/msbuild/issues/4071

ViktorHofer commented 4 years ago

@rainersigwald @AndyGerlicher @jeffkl

At this time, it is not possible to do this. The logic for /restore is pretty proprietary to MSBuild.exe while the task is using a completely different code path. That said, anything is possible, however we do not currently have plans to implement this.

Are we now, three years later, in a better position to support this? I just stumbled upon this as I was trying to avoid an unnecessary glob (which matters in large repos like dotnet/runtime).

rainersigwald commented 4 years ago

@ViktorHofer No, the essential complexity remains. Can you elaborate on your "unnecessary glob" scenario? I don't see how that ties in to this.

miloszkukla commented 1 year ago

@jeffkl @AndyGerlicher why is setting IsRestoring property needed? Where is it used?

jeffkl commented 1 year ago

@miloszkukla MSBuild caches the evaluation of a project (all of the properties and items, etc) in memory for efficiency purposes. When a restore is run, MSBuild evaluates everything first, NuGet reads everything, and runs a restore. The restore itself can inject build logic and alter properties and items. You don't want the project evaluation (all of properties and items, etc) from before the restore to be used during the actual build because the restore might have made the project buildable. To work around this, you must use a new evaluation of a project for the build. The caching mechanism in MSBuild is based on the set of global properties. So if you load a project with a global property "PropertyA=One" and then the same project with "PropertyA=Two", MSBuild will evaluate everything twice. If you load the same project with the same global properties, the same property and items values are just fetched from the cache.

So setting any random global property for the restore is just a way to make sure that the evaluation of those properties is not re-used by the build and instead a new evaluation is run.

meerfrau commented 11 months ago

(When I try to build any other project my msbuild wants to restore the gdal/build/swig/csharp build which no longer exists.)

MSBuild caches the evaluation of a project (all of the properties and items, etc) in memory for efficiency purposes. When a restore is run, MSBuild evaluates everything first [..]

@jeffkl Where are these settings stored and how may I completely reset the projects to restore?