msbuild icon indicating copy to clipboard operation
msbuild copied to clipboard

Add net481 as a netframework version

Open benvillalobos opened this issue 2 years ago • 1 comments

Fixes ToolLocationHelper returning null when on net481.

Context

In ARM64 scenarios, customers will be on net481. Calling Microsoft.Build.Utilities.ToolLocationHelper.GetPathToDotNetFramework(TargetDotNetFrameworkVersion.VersionLatest); returns null.

Changes Made

Testing

Notes

benvillalobos avatar Jun 08 '22 19:06 benvillalobos

Status on this: the arm64 net481 install does not automatically come with MSBuild.exe. This results in MSBuild almost finding the correct MSBuild, but not seeing MSBuild.exe there and therefore assuming it was a broken install.

https://github.com/dotnet/msbuild/blob/7a0fefe241f1040c4ebfa42626a854839fa0f71e/src/Shared/FrameworkLocationHelper.cs#L1383-L1386

You'll notice there's a this._hasMSBuild check, this is always set to true no matter what: https://github.com/dotnet/msbuild/blob/7a0fefe241f1040c4ebfa42626a854839fa0f71e/src/Shared/FrameworkLocationHelper.cs#L1132-L1143

Brain Dump Time

  • Can we just ship MSBuild.exe into net481?

    • Sure, but we'd still need to account for 481 installs that don't include MSBuild.exe
  • Does it really mean that the net framework install is broken if msbuild.exe doesn't exist? Clearly not.

    • Would callers of ToolLocationHelper.GetPathToDotNetFramework expect MSBuild.exe to exist? The API summarizes it as: Get a fully qualified path to the frameworks root directory., so I don't think that's a problem.
  • We can check for Microsoft.Build.dll instead of, or as well as MSBuild.exe

    • Is that dll always guaranteed to exist if msbuild.exe doesn't? I think it's safe to say that if we once assumed MSBuild.exe was going to exist, we can even more safely assume that Microsoft.Build.dll will exist as an AnyCPU binary (correct me if I'm wrong)

      • Correcting myself: Previous versions within C:/Windows/Microsoft.NET/Framework64 sometimes don't have Microsoft.Build.dll (v2.0.5, v3.5). Can we expect future versions to have this?

Current Path Forward

If MSBuild.exe doesn't exist, check if Microsoft.Build.dll exists. We can handle future cases as they come up, just like this one.

benvillalobos avatar Jun 21 '22 17:06 benvillalobos

@BenVillalobos, your comment from June looks stale. Is this still needed? Close it?

Forgind avatar Oct 06 '22 21:10 Forgind

Related issue: https://github.com/dotnet/msbuild/issues/8027

benvillalobos avatar Oct 20 '22 16:10 benvillalobos

After refreshing myself on this, The issue is that we're not shipping arm64 MSBuild.exe into C:\Windows\Microsoft.NET\FrameworkArm64\v4.0.30319.

ARM64 msbuild probably wasn't a thing when this folder was created, but it is now and we should ship msbuild.exe in there. I'm not entirely sure who to contact here. cc @marcpopMSFT

Considering the workaround I mentioned higher up in the comments (to check for Microsoft.Build.dll OR msbuild.exe), it might be worth doing if users who try to find this folder don't also expect the exe to be there, but we should tackle the real problem, which is that msbuild isn't being shipped here.

☝️ thinking more on this last sentence. If MSBuild.exe not existing here doesn't entirely mean a broken install. What is Microsot.Build.dll / most other files exist? Is it still worth returning the path? @rainersigwald thoughts?

benvillalobos avatar Nov 01 '22 19:11 benvillalobos

After discussing with Nikola and Rainer, we decided to broaden the MSBuild.exe check.

benvillalobos avatar Nov 01 '22 21:11 benvillalobos

Have you validated that this works in the container scenario that was failing?

not sure about a container scenario, but I'm going to verify locally that this scenario is fixed: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1541603/

TL;DR (for the repro): Call toollocationhelper.GetPathToDotnetFramework, expect a non-null-actual-path

benvillalobos avatar Nov 03 '22 17:11 benvillalobos

I finally got the method to return the proper path. I need to investigate what changes in the PR are actually necessary.

cc @rainersigwald about the container scenario to check afterward.

benvillalobos avatar Nov 04 '22 23:11 benvillalobos

Quick Recap: I figured out that the fix for the original issue just needs this one commit: 88dbe574b797b45cf0676c34a201b6b757a9934d.

I don't think the issue Rainer mentioned would be resolved by this. My current understanding is that we need another change similar to https://github.com/dotnet/msbuild/pull/4487/files, but I can't validate the registry entry.

benvillalobos avatar Nov 10 '22 00:11 benvillalobos

Okay, I've validated the registry entry (after realizing this was stored in the 32 bit registry, and that the 32 bit registry is separate from the 64 bit registry /shakefist).

It seems easy enough to update 4.8 to 4.8.1 similar to https://github.com/dotnet/msbuild/pull/4487/files. My main concern was "was if a user doesn't have 481 but they have 48?" My latest commit (235b524) captures that scenario by adding an explicit fallback rule.

benvillalobos avatar Nov 10 '22 21:11 benvillalobos

Just verified we can find net481, and a tool under that folder (I manually replaced Microsoft.Build.Utilities in bin/debug and reran the same code without rebuilding): image

Edit: Also verified it resolves the arm64 scenario: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1541603/

benvillalobos avatar Nov 11 '22 19:11 benvillalobos

Just verified this resolves https://github.com/dotnet/msbuild/issues/8027

See https://github.com/dotnet/msbuild/issues/8027#issuecomment-1312273773 for the details there.

benvillalobos avatar Nov 11 '22 23:11 benvillalobos

My last compatibility question here is: Should we add versions for net481 on VS 16,15, etc.? How far back do we go?

benvillalobos avatar Nov 14 '22 19:11 benvillalobos

I added the fallback logic if the net481 registry key doesn't exist. It should fall back to net48 (the last version we knew was okay) so we can avoid regressing customers for now.

benvillalobos avatar Nov 14 '22 23:11 benvillalobos