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

Add prefer native arm64 flag #10134

Merged

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented May 15, 2024

Fixes #10060

Context

Related changes in SDK: dotnet/sdk#40895

Changes Made

Added new target and task to handle the cases when PreferNativeArm64 is specified.

  • added a task to display localized messages specified in .target files

Testing

Unit tests + e2e that check that modified manifest is embedded in the final dll.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Flushing comments after the first pass. Looks great overall!

src/Tasks/Al.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.NETFramework.CurrentVersion.props Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review May 16, 2024 13:50
src/Tasks/NETMessage.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/NETMessage.cs Outdated Show resolved Hide resolved
src/Tasks/NETMessage.cs Outdated Show resolved Hide resolved
@ladipro
Copy link
Member

ladipro commented May 23, 2024

I have found a couple of issues when testing this PR in VS end to end.

  1. The application manifest template in VS contains this XML:
  <application xmlns="urn:schemas-microsoft-com:asm.v3">
    <windowsSettings>
      <dpiAware xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">true</dpiAware>
      <longPathAware xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">true</longPathAware>
    </windowsSettings>
  </application>

The PopulateSupportedArchitectures task is failing on it with:

      The "PopulateSupportedArchitectures" task failed unexpectedly.
      This is an unhandled exception from a task -- PLEASE OPEN A BUG AGAINST THE TASK OWNER.
      System.Xml.XmlException: The prefix '' cannot be redefined from 'urn:schemas-microsoft-com:asm.v3' to 'http://schemas.microsoft.com/SMI/2024/WindowsSe
      ttings' within the same start element tag.
         at System.Xml.XmlWellFormedWriter.PushNamespaceExplicit(String prefix, String ns)
         at System.Xml.XmlWellFormedWriter.WriteEndAttribute()
         at System.Xml.XmlElement.WriteStartElement(XmlWriter w)
         at System.Xml.XmlElement.WriteElementTo(XmlWriter writer, XmlElement e)
         at System.Xml.XmlDocument.Save(XmlWriter w)
         at Microsoft.Build.Tasks.PopulateSupportedArchitectures.SaveManifest(XmlDocument document, String manifestName) in C:\src\msbuild\src\Tasks\Populat
      eSupportedArchitectures.cs:line 154
         at Microsoft.Build.Tasks.PopulateSupportedArchitectures.Execute() in C:\src\msbuild\src\Tasks\PopulateSupportedArchitectures.cs:line 125
         at Microsoft.Build.BackEnd.TaskExecutionHost.Execute() in C:\src\msbuild\src\Build\BackEnd\TaskExecutionHost\TaskExecutionHost.cs:line 587
         at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext() in C:\src\msbuild\src\Build\BackEnd\Components\RequestBuilder\Task
      Builder.cs:line 807
  1. Adding <PreferNativeArm64>true</PreferNativeArm64> to an otherwise default .NET Framework console app makes it fail with MSB9902 because Prefer32Bit is enabled by default. I wonder if setting PreferNativeArm64 shouldn't disable the Prefer32Bit auto opt-in. I.e. developers would get MSB9902 only if they set Prefer32Bit explicitly in their project file.

@YuliiaKovalova
Copy link
Member Author

YuliiaKovalova commented May 23, 2024

I have found a couple of issues when testing this PR in VS end to end.

  1. The application manifest template in VS contains this XML:
  <application xmlns="urn:schemas-microsoft-com:asm.v3">
    <windowsSettings>
      <dpiAware xmlns="http://schemas.microsoft.com/SMI/2005/WindowsSettings">true</dpiAware>
      <longPathAware xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">true</longPathAware>
    </windowsSettings>
  </application>

The PopulateSupportedArchitectures task is failing on it with:

      The "PopulateSupportedArchitectures" task failed unexpectedly.
      This is an unhandled exception from a task -- PLEASE OPEN A BUG AGAINST THE TASK OWNER.
      System.Xml.XmlException: The prefix '' cannot be redefined from 'urn:schemas-microsoft-com:asm.v3' to 'http://schemas.microsoft.com/SMI/2024/WindowsSe
      ttings' within the same start element tag.
         at System.Xml.XmlWellFormedWriter.PushNamespaceExplicit(String prefix, String ns)
         at System.Xml.XmlWellFormedWriter.WriteEndAttribute()
         at System.Xml.XmlElement.WriteStartElement(XmlWriter w)
         at System.Xml.XmlElement.WriteElementTo(XmlWriter writer, XmlElement e)
         at System.Xml.XmlDocument.Save(XmlWriter w)
         at Microsoft.Build.Tasks.PopulateSupportedArchitectures.SaveManifest(XmlDocument document, String manifestName) in C:\src\msbuild\src\Tasks\Populat
      eSupportedArchitectures.cs:line 154
         at Microsoft.Build.Tasks.PopulateSupportedArchitectures.Execute() in C:\src\msbuild\src\Tasks\PopulateSupportedArchitectures.cs:line 125
         at Microsoft.Build.BackEnd.TaskExecutionHost.Execute() in C:\src\msbuild\src\Build\BackEnd\TaskExecutionHost\TaskExecutionHost.cs:line 587
         at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext() in C:\src\msbuild\src\Build\BackEnd\Components\RequestBuilder\Task
      Builder.cs:line 807
  1. Adding <PreferNativeArm64>true</PreferNativeArm64> to an otherwise default .NET Framework console app makes it fail with MSB9902 because Prefer32Bit is enabled by default. I wonder if setting PreferNativeArm64 shouldn't disable the Prefer32Bit auto opt-in. I.e. developers would get MSB9902 only if they set Prefer32Bit explicitly in their project file.

For the second point it would require special handling in VS by project systems - where on checking PreferNativeArm64
, Prefer32Bit has to be handled automatically. Can we solely decide it here?

The first one was fixed here: 170409a

src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/NETMessage.cs Outdated Show resolved Hide resolved
src/Tasks/Resources/default.win32manifest Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
src/Tasks/PopulateSupportedArchitectures.cs Outdated Show resolved Hide resolved
@ladipro
Copy link
Member

ladipro commented May 24, 2024

3. Adding <PreferNativeArm64>true</PreferNativeArm64> to an otherwise default .NET Framework console app makes it fail with MSB9902 because Prefer32Bit is enabled by default. I wonder if setting PreferNativeArm64 shouldn't disable the Prefer32Bit auto opt-in. I.e. developers would get MSB9902 only if they set Prefer32Bit explicitly in their project file.

For the second point it would require special handling in VS by project systems - where on checking PreferNativeArm64 , Prefer32Bit has to be handled automatically. Can we solely decide it here?

I believe we do plan to implement some of this logic in Visual Studio project properties UI.

  • Prefer native ARM64 should be automatically unchecked and disabled when platform target is not Any CPU.
  • Checking Prefer native ARM64 when Prefer 32-bit is checked should uncheck Prefer 32-bit, and vice versa.

So things should work well in VS because VS adds <Prefer32Bit>false</Prefer32Bit> when the ARM64 checkbox is checked. My concern is with modifying the project outside of VS. Say that the user just adds <PreferNativeArm64>true</PreferNativeArm64> and then the project fails to build complaining about PreferNativeArm64 and Prefer32Bit both set. However, the user doesn't have Prefer32Bit in their project file, so they may find the error confusing.

My proposal is to update the condition here to test if PreferNativeArm64 is set:

<PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(TargetingClr2Framework)' != 'true' and '$(TargetFrameworkVersion)' != 'v4.0' and ('$(OutputType)' == 'exe' or '$(OutputType)' == 'winexe' or '$(OutputType)' == 'appcontainerexe' or '$(OutputType)' == '')">

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, I am leaving a few more comments inline.

src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Microsoft.NETFramework.CurrentVersion.props Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.tasks Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/AddToWin32Manifest.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/add_PreferNativeArm64 branch from 9f04806 to bdb3aba Compare May 24, 2024 12:25
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Thank you!

src/Tasks/Microsoft.NETFramework.CurrentVersion.props Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/AddToWin32Manifest.cs Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/AddToWin32Manifest.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.tasks Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova merged commit 7ae92ce into dotnet:main May 29, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Implement support for native ARM64 enablement in NetFx AnyCPU executables
5 participants