Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
natemcmaster opened this issue Dec 15, 2017 · 18 comments
Open

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

natemcmaster opened this issue Dec 15, 2017 · 18 comments

Comments

@natemcmaster
Copy link
Contributor

#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
Copy link
Contributor

jeffkl commented Dec 15, 2017

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
Copy link
Contributor Author

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

@AndyGerlicher
Copy link
Contributor

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

@natemcmaster
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

We had a bug for that I just ran across: #1185, including a case where the "snapshot" behavior is required for correctness.

@msshapira
Copy link

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

@PhilipDaniels
Copy link

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 #3000 (comment) 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
Copy link

@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
Copy link
Member

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
Copy link

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 #4071

ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue Jan 14, 2020
The restore and build target invocations need to happen in separate
msbuild task invocations and different global properties need to be
passed in to mitigate issues with caching the invocations. For more
context see dotnet/msbuild#2811.
@ViktorHofer
Copy link
Member

@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
Copy link
Member

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

jonpryor added a commit to dotnet/android that referenced this issue Mar 27, 2020
Changes: dotnet/java-interop@1a086ff...56c92c7

  * dotnet/java-interop@56c92c7: [build] Remove cecil submodule (#597)
  * dotnet/java-interop@3091274: [build] Provide a default $(Configuration) value (#612)
  * dotnet/java-interop@cf3e7c2: [generator] Don't process duplicate reference assemblies (#611)
  * dotnet/java-interop@f5fa462: [jnienv-gen] Convert to SDK-style (#608)

Of particular note is [dotnet/java-interop@56c92c7][0], which
replaces the `mono/cecil` submodule within Java.Interop with the
[`Mono.Cecil` NuGet package][1] in an effort to simplify the
Java.Interop build system.

This simplifies the Java.Interop repo, and we *thought* that since
xamarin-android *doesn't even use* Java.Interop's cecil submodule-built
`Mono.Cecil.dll` -- instead the `Mono.Cecil.dll` from the
"mono archive" is "renamed" to `Xamarin.Android.Cecil.dll` during
`make prepare` (0c9f83b) -- surely this would be a simple change.

The removal of the cecil submodule also required changing
`ThirdPartyNotice.txt` generation so that the LICENSE for Cecil was
obtained from the mono archive instead of from Java.Interop.

Unfortunately, the integration was a tad more complicated than
anticipated.  With the ongoing adoption of MSBuild multi-targeting
and builds against the `netcoreapp3.1` target framework -- commit
e2854ee and numerous commits in Java.Interop -- we encountered a
problem with MSBuild semantics: If two `$(TargetFramework)` builds
share the same output directory, the `IncrementalClean` target will
*remove files created by previous builds*, e.g. when e.g.
`Java.Interop/tools/generator.csproj` builds the `netcoreapp3.1`
framework, it will *delete* the `generator.exe` built by the `net472`
framework, which results in subsequent build breaks.

The only path to sanity is to *ensure* that different
`$(TargetFramework)` builds have *completely separate* `$(OutputPath)`
values.  The "normal" approach to doing this is for `$(OutputPath)`
to end with `$(TargetFramework)`, which is the case when
`$(AppendTargetFrameworkToOutputPath)`=True (the default).

Unfortunately in xamarin-android we don't want `$(OutputPath)` to end
with `$(TargetFramework)`; we want the build tree structure to mirror
the installation directory structure, which -- at present -- doesn't
mention `$(TargetFramework)` at all.

The solution here is to use "non-overlapping" directories.  For
example, in e2854ee there are "two" `$(OutputPath)` values:

  * `MonoAndroid10.0`: `bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0/Mono.Android.dll`
  *   `netcoreapp3.1`: `bin/Debug/lib/xamarin.android/xbuild-frameworks/Xamarin.Android.App/netcoreapp3.1/Mono.Android.dll`

The same "non-overlapping directories" strategy needs to be extended
to *all* multi-targeted projects from Java.Interop, *including*
dependencies.  Dependencies such as `Xamarin.Android.Cecil.dll`.

Define a new `$(UtilityOutputFullPathCoreApps)` MSBuild property so
that Java.Interop "utility" project builds, when building for the
`netcoreapp3.1` framework, use a *different* `Xamarin.Android.Cecil.dll`
than is used with the `net472`-related builds.

Update `xaprepare` to *create* this new `netcoreapp3.1`-correlated
`Xamarin.Android.Cecil.dll`.  It's the same file, just in a different
directory, to prevent "accidental" deletes by `IncrementalClean`.

Even with all that, MSBuild still had other ideas.  In particular,
MSBuild wasn't particularly happy about our attempt to use the
`$(UtilityOutputFullPath)` property to "switch" between using a
`@(PackageReference)` to the Mono.Cecil NuGet package vs. using a
`@(Reference)` to the `Xamarin.Android.Cecil.dll` assembly, because
MSBuild *caches* this information somewhere within `obj` directories.

To get MSBuild to re-evaluate it's assembly reference choices, we must
instead replace `msbuild` with `msbuild /restore`.

Which still isn't enough, because some of our MSBuild invocations are
via the `<MSBuild/>` task, within `msbuild`.  To get *that* working,
we need to explicitly invoke the `Restore` target through a *separate*
`<MSBuild/>` task invocation.  You ***CANNOT*** use
`<MSBuild Targets="Restore;Build" />`, as "obvious" as that may be,
because it [doesn't work reliably][2].  ([Yet.][3])

[0]: dotnet/java-interop@56c92c7
[1]: https://www.nuget.org/packages/Mono.Cecil/0.11.2
[2]: dotnet/msbuild#3000 (comment)
[3]: dotnet/msbuild#2811
jonpryor pushed a commit to dotnet/java-interop that referenced this issue Jan 24, 2022
`dotnet build Java.Interop.sln` didn't work unless `-m:1` was used:

	% make prepare-core JI_MAX_JDK=8
	% dotnet build -target:Prepare -p:MaxJdkVersion=8 Java.Interop.sln
	% dotnet build Java.Interop.sln
	…
	…/tests/Java.Interop.Export-Tests/Java.Interop.Export-Tests.csproj(35,5): error MSB3073: The command ""/Users/jon/android-toolchain/jdk-1.8/bin/javac" -classpath …" exited with code 1.
	…/tests/Java.Interop.Export-Tests/Java.Interop.Export-Tests.csproj(35,5): error MSB3073: The command ""/Users/jon/android-toolchain/jdk-1.8/bin/javac" -classpath …" exited with code 1.
	…/tests/Java.Interop-Tests/Java.Interop-Tests.csproj(60,5): error MSB3073: The command ""/Users/jon/android-toolchain/jdk-1.8/bin/javac" -source 1.8 -target 1.8 -bootclasspath …" exited with code 1.
	…/tests/Java.Interop-Tests/Java.Interop-Tests.csproj(60,5): error MSB3073: The command ""/Users/jon/android-toolchain/jdk-1.8/bin/javac" -source 1.8 -target 1.8 -bootclasspath …" exited with code 1.

Using `dotnet build -m:1` avoids the above MSB3073 errors; see also
commit 1de5501, which added `-m:1` to CI builds.

Fix these parallel build issues so that `-m:1` isn't needed.  This is
principally done by removing `Java.Interop.BootstrapTasks.csproj`
from `Java.Interop.sln`, and adding it to a new
`Java.Interop.BootstrapTasks.sln` solution.  The `Prepare` target is
updated to explicitly Restore and Build the new
`Java.Interop.BootstrapTasks.dll` solution.

Note that the Restore and Build target invocations must be separate;
using [`Targets="Restore;Build"` will not work][0].

Update `Java.Interop.BootstrapTasks.csproj` to *no longer* `<Import/>`
`VersionInfo.targets`, as this causes a circular dependency
(`VersionInfo.targets` uses tasks from
`Java.Interop.BootstrapTasks.dll`).  This is fine, as
`Java.Interop.BootstrapTasks.dll` doesn't need to be versioned.

Review our custom `.targets` files, and replace
`BeforeTargets="BeforeBuild"` with `BeforeTargets="Build"` or
`BeforeTargets="BeforeCompile"` (as appropriate).
Building before the `BeforeBuild` target is problematic, as the
`BeforeBuild` target runs before `@(ProjectReference)`d projects are
built, meaning dependencies may not exist.  (Oops.)

TODO: Instead of having two `.sln` files, try to instead use
`<UsingTask/>` with `AssemblyFile` and `TasFactory` set, as done in
[rainersigwald/build-task-in-solution-demo][1] and the
[`InvokeCustomTaskDemo` target][2].  This *may* avoid the need for
a separate `.sln` (and `Prepare` target invocation?).

[0]: dotnet/msbuild#2811 (comment)
[1]: https://github.com/rainersigwald/build-task-in-solution-demo
[2]: https://github.com/rainersigwald/build-task-in-solution-demo/blob/17508937bb9edb0b817820146f85adf0da331515/Task/DemoFunctionality.targets#L4-L10
@miloszkukla
Copy link

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

@jeffkl
Copy link
Contributor

jeffkl commented Jan 4, 2023

@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
Copy link

meerfrau commented Jul 14, 2023

(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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests