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

[Built-in analyzer] Flag shared evaluations between restore and build targets #9553

Open
ViktorHofer opened this issue Dec 15, 2023 · 3 comments
Labels
Area: BuildCheck backlog BuildCheck Suggestion Suggestion for a built in MSBuild analyzer. Label should be applied together with 'Area: BuildCheck' Feature: Warning Waves Warnings to enable in opt-in waves. Formerly "strict mode". triaged

Comments

@ViktorHofer
Copy link
Member

See https://github.com/dotnet/runtime/blob/6d0a902c9585d98bfa44f514bac21a47eabe02fa/eng/testing/workloads-testing.targets#L195-L200 as an example.

Because the Restore and Pack target pass the same set of properties in, the underlying project evaluation evaluation is shared. This resulted in msbuild files restored via a PackageReference not being imported during the build.

I have been fixing so many of these code pieces in my career at Microsoft and it would really be helpful if msbuild could somehow indicate that a shared evaluation between Restore and Build or Pack is most certainly wrong. Sure, there are cases where a shared evaluation might be intentional, but that should < 1%.

Related: #2811

cc @akoeplinger @ericstj

@rainersigwald rainersigwald added the Feature: Warning Waves Warnings to enable in opt-in waves. Formerly "strict mode". label Dec 15, 2023
@rainersigwald
Copy link
Member

The error should be clear that just having different evaluations is not enough, they have to be in separate executions.

@lambdageek
Copy link
Member

different evaluations is not enough, they have to be in separate executions.

@rainersigwald I'm a bit unclear on what that means. Does that mean that this is not a valid workaround?

<Target Name="RestoreBuildAndPublishSubproject">
        <MSBuild Projects="SomeSubproject.csproj"
                 Properties="Configuration=$(NativeLibsPublishConfiguration);RuntimeIdentifier=$(OutputRID);NativeLib=$(NativeLibKind);_RandomPropertyToPleaseMSBuild=Restoring"
                 Targets="Restore" />

        <MSBuild Projects="SomeSubproject.csproj"
                 Properties="Configuration=$(NativeLibsPublishConfiguration);RuntimeIdentifier=$(OutputRID);NativeLib=$(NativeLibKind);_RandomOtherPropertyToPleaseMSBuild=BuildingAndPublishing"
                 Targets="Build;Publish" />
</Target>

@rainersigwald
Copy link
Member

@lambdageek correct. The failure mode is:

  1. The project has references that include MSBuild logic (build/PackageName.props).
  2. Restore.
  3. Change something such that the packages that include the MSBuild logic are changed (bump the patch version of a reference, say).
  4. Execute the Restore;Build or something like the workaround you posted.

Inside 4, the timeline goes like

  1. Evaluate projects, including the .nuget.g.props file that was created in 3.
  2. Do restore stuff, download the packages, whatever.
  3. As part of that restore, update .nuget.g.props on disk to point to the new package version.
  4. Enter the Build part.
    a. If it was via Restore;Build this reuses the evaluation from 1, so still uses the old package version.
    b. If there's a new global property, the MSBuild engine reevaluates the project, but because it's part of the same build session it has an internal cache of the XML of all imports, when it reprocesses .nuget.g.props it's still the old version.

We considered allowing invalidating the XML cache in that scenario but it turns out to be load-bearing for some cases including Live Unit Testing and some builds that try to bump their own version mid-build.

Things work most of the time because it's not super common to bump the versions of packages that have build logic . . . but that's not 100%.

@baronfel baronfel changed the title Introduce analyzer or some other mechanism to flag shared evaluations between restore and build targets [Built-in analyzer] Flag shared evaluations between restore and build targets Jun 19, 2024
@baronfel baronfel added the BuildCheck Suggestion Suggestion for a built in MSBuild analyzer. Label should be applied together with 'Area: BuildCheck' label Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BuildCheck backlog BuildCheck Suggestion Suggestion for a built in MSBuild analyzer. Label should be applied together with 'Area: BuildCheck' Feature: Warning Waves Warnings to enable in opt-in waves. Formerly "strict mode". triaged
Projects
None yet
Development

No branches or pull requests

6 participants