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

force set Condition="false" on Microsoft.WebApplication.targets #8946

Merged
merged 7 commits into from
Feb 6, 2024

Conversation

brettfo
Copy link
Collaborator

@brettfo brettfo commented Jan 31, 2024

The import $(VSToolsPath)\WebApplications\Microsoft.WebApplications.targets can never be resolved in the updater as it runs in the Linux Docker container. Under most conditions, the $(VSToolsPath) property will not be set so this isn't an issue (n.b., the Condition="'$(VSToolsPath)' != ''" attribute), but some project files do set it, which makes it impossible to skip importing that targets file through the normal means.

The fix in this PR is to always set Condition="false" on that import node and then restore it after the run to its original state. An equally valid fix would have been to remove/re-add the entire <Import> node, but re-inserting the original node at the correct location is difficult; instead it's much easer to find the node and modify (or remove, etc.) the condition attribute.

It is also a possibility for the code owners to update their .csproj to use the SDK-friendly MSBuild.Microsoft.VisualStudio.Web.targets NuGet package. That would allow the updater to still run, but requires non-trivial permanent changes/migrations to their project file and that's outside the scope of what we're trying to accomplish: an in-place package update with a minimal diff.

Another potential fix would be to place an empty Microsoft.WebApplication.targets file in the Docker image. This would certainly work at runtime and require less code, but would not be unit-testable.

I split up the behavior into parts that I thought made the most sense.

  1. Only the smallest call into our NuGet runner is wrapped with the WebApplicationTargetsConditionPatcher. I used IDisposable for everything to ensure the original file is always restored to its correct state.
  2. The web application patcher only knows how to find the requisite <Import> node and set/reset the condition attribute, nothing more.
  3. That patcher builds on top of a more generic XmlFilePreAndPostProcessor class. This is the one that parses the XML and calls the appropriate PreProcess and PostProcess functions.

All of the code was done this way so that if we have to expand the pattern of targets files that can't be imported, we can do very targeted fixes with the minimal amount of mess. Ideally this will be the only special case we need, but in all reality, we may end up with one or two more in the future.

Fixes #8532.

@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Jan 31, 2024
The file $(VSToolsPath)\WebApplications\Microsoft.WebApplications.targets can never be resolved in the updater as
it runs in the Linux Docker container, so an always false condition is added to that import to make the minimal change necessary to allow the NuGet updater to successfully run.  After completion, the condition attribute is restored.
@brettfo brettfo marked this pull request as ready for review January 31, 2024 23:42
@brettfo brettfo requested a review from a team as a code owner January 31, 2024 23:42
private string? _capturedCondition;
private readonly XmlFilePreAndPostProcessor _processor;

public WebApplicationTargetsConditionPatcher(string projectFilePath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this shape, I think it makes it relatively easy to create a couple of similar Patchers and group them together into an aggregator.

@abdulapopoola abdulapopoola merged commit 182ee68 into main Feb 6, 2024
69 checks passed
@abdulapopoola abdulapopoola deleted the dev/brettfo/package-update branch February 6, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.WebApplication.targets was not found
4 participants