dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.67k stars 1.06k forks source link

NativeAOT publish for DLL does not set automatically RID #29834

Open kant2002 opened 1 year ago

kant2002 commented 1 year ago

I was have following setup. DLL project which I publish as NativeAOT. I have to publish it dotnet publish -r win-x64 --self-contained otherwise project does not pickup nor current RID of my dev machine, not the fact that application is self-contained (NativeAOT). It's most likely issue in SDK

ghost commented 1 year ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Issue Details
I was have following setup. DLL project which I publish as NativeAOT. I have to publish it `dotnet publish -r win-x64 --self-contained` otherwise project does not pickup nor current RID of my dev machine, not the fact that application is self-contained (NativeAOT). It's most likely issue in SDK - `dotnet publish` want me specify RID - `dotnet publish -r win-x64` use WinForms references during build, but do not propagate them for publish to ILC - `dotnet publish -r win-x64 --self-contained` works, but `self-contained` switch is misleading probably and undo a lot of tooling work in recent releases around NativeAOT
Author: kant2002
Assignees: -
Labels: `untriaged`, `area-NativeAOT-coreclr`
Milestone: -
am11 commented 1 year ago

In .NET 7+, dotnet-publish sets the RID implicitly to the runtime of executing dotnet.exe with Publish{Aot,SingleFile,ReadyToRun}.

You can also pass --use-current-runtime or --ucr in a general-purpose script (e.g. for CI) for cases which aren't covered by implicit RID. I don't remember offhand which are those cases, @nagilson might.

dotnet new mvc -n mvc1 && dotnet publish -p:PublishAot=true mvc1 publishes AOT app for win-x64 on Windows 10 x64 machine; without explicitly specifying the runtime with .NET 7 (and 8 alpha).

kant2002 commented 1 year ago

I know about that, but if you create

dotnet new winforms 
# remove OutputType
dotnet publish

then you will have situation like I describe. Probably MVC is special cased somehow.

am11 commented 1 year ago

Well, I tested with two templates: Console and MVC. WinForms is rather an exception.

dotnet new winforms 
# remove OutputType
dotnet publish

I don't see PublishAot specified anywhere.

kant2002 commented 1 year ago

You may take a look at this project https://github.com/kant2002/WinFormsComInterop/blob/main/samples/HostedWindowsForms/HostedWindowsForms.csproj

which was led to following error https://github.com/kant2002/WinFormsComInterop/issues/48#issue-1504667504 if using dotnet publish -r win-64.

kant2002 commented 1 year ago

I think issue is specifically for OutpuType=Library kind of projects. That maybe by design in regular CoreCLR publish, but unexpected for NativeAOT

am11 commented 1 year ago

Yes, classlib requires the runtime argument:

$ dotnet new classlib -n lib1
$ dotnet publish lib1 -p:PublishAot=true --use-current-runtime
kant2002 commented 1 year ago

I see two problems here, why PublishAot does not imply --use-current-runtime and second. Why PublishAot does not imply --self-contained. I may probably understand some rationale for first case, but not for second.

am11 commented 1 year ago

Your sample https://github.com/kant2002/WinFormsComInterop/tree/main/samples/HostedWindowsForms is building on my Win10 box with .NET SDK 7.0.101 without --self-contained:

dotnet publish --use-current-runtime
::: or
dotnet publish -r win-x64

...
Generating native code
...

produces bin\Debug\net7.0-windows\win-x64\publish\HostedWindowsForms.dll which is a native library according to ildasm.exe.

kant2002 commented 1 year ago

I belive you are did not notice ILC: Method '[HostedWindowsForms]HostedWindowsForms.LibraryFunctions.ShowMessageBox()' will always throw because: Fai led to load assembly 'System.Windows.Forms' . that's because System.Windows.Forms.dll is not part of set of published files. And that's because dotnet SDK think this is not self-contained app.

am11 commented 1 year ago

ILC: Method '[HostedWindowsForms]HostedWindowsForms.LibraryFunctions.ShowMessageBox()' will always throw because: Fai led to load assembly 'System.Windows.Forms'

Sounds like this should be an error.

kant2002 commented 1 year ago

ILC reporting is separate issue. I'm worry only about SDK behavior. It seems reasonable that if PublishAot=true then SDK infer RID and --self-contained independently from OutputType of the project.

MichalStrehovsky commented 1 year ago

This is probably because publishing a library mostly works by accident in the SDK and wasn't fully tested for everything. https://github.com/dotnet/linker/issues/3165#issuecomment-1369979182. Looks like it doesn't even work for all NativeAOT scenarios (although WinForms with NativeAOT is definitely not a supported scenario in 7.0).

ILC: Method '[HostedWindowsForms]HostedWindowsForms.LibraryFunctions.ShowMessageBox()' will always throw because: Fai led to load assembly 'System.Windows.Forms'

Sounds like this should be an error.

Nope, people add assemblies their grandmother wrote for .NET Framework 1.0 to their .NET 7 apps and expect the parts that work with a JIT-based runtime to also work on an AOT-based runtime. (I.e. publishing apps with missing or mismatched dependencies is common so the compiler makes a reasonable attempt to emulate the JIT behavior where one would get an exception only once a problematic codepath is reached.)

kant2002 commented 1 year ago

So we have one more case in addition to Android where publish self-contained libraries make sense. Is this should be better documented in sample, or SDK work?

MichalStrehovsky commented 1 year ago

It already mostly works for NativeAOT. Did you hit it in a scenario that doesn't involve WinForms? We don't document/make samples for things that are unsupported.

(I assume the problem is that WinForms adds itself to publish differently than regular NuGet packages and we need a fix in the .NET SDK or WinForms SDK to do it in a way that works with NativeAOT. Basically the SDKs don't expect publishing as libraries and may need extra work.)

kant2002 commented 1 year ago

Does failure of embed of ASP.NET Core count as "supported" scenario? I test it and it produce ILC: Method '[webembedded]LibraryFunctions.ShowMessageBox()' will always throw because: Failed to load assembly 'Microsoft.AspNetCore'. Do you need repo with repro?

MichalStrehovsky commented 1 year ago

ASP.NET Core is also not supported with NativeAOT. It's only a .NET 7 servicing-worthy issue if this doesn't work with regular NuGets. Otherwise this would be "help-wanted", milestoned as "Future".

kant2002 commented 1 year ago

I think it affects only cases where library distinguish between self-contained and not self-contained modes. I do not aware of non-MS and popular enough Sdks which do this. So be it -"help wanted" and "Future". Any chance that sdk repo(am I right?) would accept contributions in this area? Once ASP.NET Core make significant progress in NativeAOT I think this issue will re-appear, since I bet some people try to put it in the unmanaged app.

am11 commented 1 year ago

If most (or all) of the PublishAot=true cases must imply SelfContained=true ignoring the project OutputType, then this should be fine: https://github.com/am11/sdk/commit/dc23244c6a5fd0bddbd24ebab895402be1bc8f0f (props are still overridable if someone really needs SelfContained=false).

vitek-karas commented 1 year ago

NativeAOT has the unwanted privilege of being basically the first scenario in which library project needs to be published self-contained. (publishing library project itself is somewhat undefined operation in SDK, making it self-contained is just making it worse). Android apps are like this already, but I think they go well outside the normal SDK integration, so it's not a fair comparison.

Any chance that sdk repo(am I right?) would accept contributions in this area?

I don't see why it wouldn't. I think the toughest part is actually defining what it means to do these things. And then reconciling that with existing behavior which is most likely different and thus it will be a breaking change.

nagilson commented 1 year ago

This is definitely an SDK issue, but I don't agree with the idea of turning the automatic RID on for everything... in fact it was like that during part of 7.0.100, and it broke a lot of people.

But @kant2002 is 💯 % right in that it's the fact that it's a library that's the reason there is no implicit RID. The RID only is inferred for executable projects, there are a lot of library projects: EX for a classlib, PublishReadyToRun should not build or publish successfully.

Maybe the suggestion is for winforms project types in particular we allow them to get the implicit RID? Tbh I don't know anything about winforms, but it sounds like something we'd want to do.

am11 commented 1 year ago

in fact it was like that during part of 7.0.100, and it broke a lot of people.

Were those people using PublishAot=true? This issue is specific to NativeAOT, where RID and SelfContained are always required during the publish regardless of OutputType. The proposed fix does not affect anything else.

vitek-karas commented 1 year ago

The issues were not with publish itself, they were that setting PublishAot=true in a project file affected the behavior of dotnet build and other commands which had nothing to do with publish. That's not to say that it would make sense to default a RID when publishing a library as NativeAOT.

am11 commented 1 year ago

I see, so '$(_IsPublishing)' == 'true' must be checked (if it isn't already).

That's not to say that it would make sense to default a RID when publishing a library as NativeAOT.

Could you explain why it wouldn't make sense? In my understanding, NativeAOT publishing requires RID (mandatory). We are inferring RID for !IsRidAgnostic cases in .NET 7. So I don't see a reason why we should let OutputType affect this inference with dotnet publish -p:PublishAot=true.

nagilson commented 1 year ago

The issues were not with publish itself, they were that setting PublishAot=true in a project file affected the behavior of dotnet build

That was an issue but not the one I was referring to. There was a different one where VS was broken because of library projects getting a RID, and builds working that shouldn't have worked. https://github.com/dotnet/sdk/pull/28628

Were those people using PublishAot=true?

The specific issue was with PublishSingleFile. I'm pretty sure that property and PublishReadyToRun shouldn't get an implicit RID for libraries. PublishAot auto-rid I haven't heard of breaking anyone, but I don't really understand it well enough to say. But if someone from the Aot side is able to comment on this, and confirm we would want a RID if it's in any type of library project, or which types of library projects we'd want a RID for PublishAot, that would be a change we could do.

vitek-karas commented 1 year ago

That's not to say that it would make sense to default a RID when publishing a library as NativeAOT.

Could you explain why it wouldn't make sense?

Sorry, typo. I meant to say that it does make sense to default a RID when publishing a library as NativeAOT. Lot of the complexity comes from the fact that we want to default the RID typically only when publishing. Building a library because it's a dependency of some other project should not be affected by publish properties. But unfortunately the way SDK/msbuild currently operates the RID defaulting has to happen early on. I'm definitely no expert in that area though - I'll leave it to @nagilson to comment on the feasibility of doing this. Things which I find "interesting":

kant2002 commented 1 year ago

I would like to split discussion in two parts. Seems to be for --self-contained there no ambiguity, can we just make this change, since that's what give most headache. RID discussion is important, but less impactful on dev experience IMO.

Regarding RID. My understanding that dotnet build,dotnet run and dotnet test are for inner dev loop, where dotnet publish is publish process. If I read conversation correctly, we all want this change only for publish process, without touching dev inner loop. I'm not that familiar with SDK, does RID inference affects dotnet build currently for some scenarios?

nagilson commented 1 year ago

I'll leave it to @nagilson to comment on the feasibility of doing this

ATM, we can limit the inference only by the dotnet command given: if a library needs to be 'built' but not 'published,' and the library is the dependency of something we invoked dotnet publish on, we don't have the granular control to change the library behavior, it would get the publish behavior. It may be theoretically possible to do it a different way, but it'd probably be an "Epic" in terms of work required and may be impossible.

How will that work when I dotnet build that app? What will happen if I dotnet publish the app

Currently it wouldn't get the inference. As above, we could change the behavior for build and publish on the app. But we'd need a MSBuild property that is available early on to tell the difference between a Library that's NativeAot and one that's not.

What is the RID behavior when I dotnet build the library which has PublishAot=true in its project file.

Currently it shouldn't do anything.

What about dotnet test

This I'm unsure of.

does RID inference affects dotnet build currently for some scenarios?

Before 7.0.200 it did for every property that gets inference, in 7.0.200 only SelfContained.

can we just make this change

Would need to look into self-contained a bit more to make sure nothing is broken.

dougkl commented 1 month ago

I am having the same issue, this error just shows when the project is set the PublishAot = true and FrameworkDependent if I try to set my project as SelfContained I have ILC Compile errors

08:15:35.128   1:7>C:\Users\Superusername\.nuget\packages\microsoft.windowsdesktop.app.runtime.win-x64\8.0.7\runtimes\win-x64\lib\net8.0\System.Windows.Forms.dll : warning IL3053: Assembly 'System.Windows.Forms' produced AOT analysis warnings. [E:\ProjectSr\WebServiceStarter.csproj]
08:15:35.672   1:7>C:\Users\Superusername\.nuget\packages\microsoft.windowsdesktop.app.runtime.win-x64\8.0.7\runtimes\win-x64\lib\net8.0\System.Configuration.ConfigurationManager.dll : warning IL2104: Assembly 'System.Configuration.ConfigurationManager' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries [E:\ProjectSr\WebServiceStarter.csproj]
08:15:36.170   1:7>C:\Users\Superusername\.nuget\packages\microsoft.data.sqlclient\5.1.5\runtimes\win\lib\net6.0\Microsoft.Data.SqlClient.dll : warning IL3053: Assembly 'Microsoft.Data.SqlClient' produced AOT analysis warnings. [E:\ProjectSr\WebServiceStarter.csproj]
08:15:36.734   1:7>C:\Users\Superusername\.nuget\packages\runtime.win-x64.microsoft.dotnet.ilcompiler\8.0.7\framework\System.Private.DataContractSerialization.dll : warning IL3053: Assembly 'System.Private.DataContractSerialization' produced AOT analysis warnings. [E:\ProjectSr\WebServiceStarter.csproj]
08:15:45.034   1:7>EXEC : error : One or more errors occurred. (Invalid IL or CLR metadata) [E:\ProjectSr\WebServiceStarter.csproj]
                     System.AggregateException: One or more errors occurred. (Invalid IL or CLR metadata) (TaskId:234)
                      ---> Internal.TypeSystem.TypeSystemException+InvalidProgramException: Invalid IL or CLR metadata (TaskId:234)
                        at Internal.TypeSystem.ThrowHelper.ThrowInvalidProgramException() + 0x24 (TaskId:234)
                        at Internal.JitInterface.CorInfoImpl.CompileMethodInternal(IMethodNode, MethodIL) + 0x22a (TaskId:234)
                        at Internal.JitInterface.CorInfoImpl.CompileMethod(MethodCodeNode, MethodIL) + 0x5c (TaskId:234)
                        at ILCompiler.RyuJitCompilation.CompileSingleMethod(CorInfoImpl, MethodCodeNode) + 0xc7 (TaskId:234)
                        at ILCompiler.RyuJitCompilation.CompileSingleMethod(MethodCodeNode methodCodeNodeNeedingCode) + 0x90 (TaskId:234)
                        at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion) + 0x289 (TaskId:234)
                     --- End of stack trace from previous location --- (TaskId:234)
                        at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x20 (TaskId:234)
                        at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw(Exception) + 0x13 (TaskId:234)
                        at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion) + 0x44a (TaskId:234)
                        at System.Threading.Tasks.TaskReplicator.Replica.Execute() + 0x66 (TaskId:234)
                        --- End of inner exception stack trace --- (TaskId:234)
                        at System.Threading.Tasks.TaskReplicator.Run[TState](TaskReplicator.ReplicatableUserAction`1, ParallelOptions, Boolean) + 0x150 (TaskId:234)
                        at System.Threading.Tasks.Parallel.ForWorker[TLocal,TInt](TInt, TInt, ParallelOptions, Action`1, Action`2, Func`4, Func`1, Action`1) + 0x394 (TaskId:234)
                     --- End of stack trace from previous location --- (TaskId:234)
                        at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x20 (TaskId:234)
                        at System.Threading.Tasks.Parallel.ThrowSingleCancellationExceptionOrOtherException(ICollection, CancellationToken, Exception) + 0x30 (TaskId:234)
                        at System.Threading.Tasks.Parallel.ForWorker[TLocal,TInt](TInt, TInt, ParallelOptions, Action`1, Action`2, Func`4, Func`1, Action`1) + 0x450 (TaskId:234)
                        at ILCompiler.RyuJitCompilation.CompileMultiThreaded(List`1) + 0x1f0 (TaskId:234)
                        at ILCompiler.RyuJitCompilation.ComputeDependencyNodeDependencies(List`1 obj) + 0x166 (TaskId:234)
                        at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes() + 0x95 (TaskId:234)
                        at ILCompiler.RyuJitCompilation.CompileInternal(String, ObjectDumper) + 0x1f (TaskId:234)
                        at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String, ObjectDumper) + 0x2d (TaskId:234)
                        at ILCompiler.Program.Run() + 0x2772 (TaskId:234)
                        at ILCompiler.ILCompilerRootCommand.<>c__DisplayClass227_0.<.ctor>b__0(ParseResult result) + 0x315 (TaskId:234)
08:15:45.177   1:7>C:\Users\Superusername\.nuget\packages\microsoft.dotnet.ilcompiler\8.0.7\build\Microsoft.NETCore.Native.targets(308,5): error MSB3073: The command ""C:\Users\Superusername\.nuget\packages\runtime.win-x64.microsoft.dotnet.ilcompiler\8.0.7\tools\ilc" @"obj\Release\net8.0-windows\win-x64\native\WebServiceStarter.ilc.rsp"" exited with code 1. [E:\ProjectSr\WebServiceStarter.csproj]
                   Done executing task "Exec" -- FAILED. (TaskId:234)
08:15:45.177   1:7>Done building target "IlcCompile" in project "WebServiceStarter.csproj" -- FAILED.: (TargetId:364)
08:15:45.177   1:7>Done Building Project "E:\ProjectSr\WebServiceStarter.csproj" (Publish target(s)) -- FAILED.
vitek-karas commented 1 month ago

@dougkl :

this error just shows when the project is set the PublishAot = true and FrameworkDependent

What do you want to achieve by this setup? Currently NativeAOT doesn't support anything but self-contained mode (it will always include the runtime in the final binary).

For the compiler crash (when self-contained is enabled), could you please create a new issue in the dotnet/runtime repo?

/cc @MichalStrehovsky

dougkl commented 1 month ago

@vitek-karas just did https://github.com/dotnet/runtime/issues/105947 thanks