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

Some .c and .h files are being published for non-blazorwasm apps #48016

Closed
eerhardt opened this issue Feb 8, 2021 · 21 comments
Closed

Some .c and .h files are being published for non-blazorwasm apps #48016

eerhardt opened this issue Feb 8, 2021 · 21 comments
Assignees
Labels
arch-wasm WebAssembly architecture area-Build-mono
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Feb 8, 2021

  1. Install the latest https://github.com/dotnet/installer#installers-and-binaries
  2. dotnet new blazorwasm
  3. dotnet publish -c Release

Expected Results

Only the files necessary to load the blazor app are in the publish folder

Actual Results

There are 3 .c files and 2 .h files in bin\Release\net6.0\publish\wwwroot\_framework:

  • corebindings.c
  • driver.c
  • pinvoke.c
  • pinvoke.h
  • pinvoke-table.h

image

These files shouldn't be published to the output folder.

cc @lewing @CoffeeFlux

@eerhardt eerhardt added the arch-wasm WebAssembly architecture label Feb 8, 2021
@ghost
Copy link

ghost commented Feb 8, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  1. Install the latest https://github.com/dotnet/installer#installers-and-binaries
  2. dotnet new blazorwasm
  3. dotnet publish -c Release

Expected Results

Only the files necessary to load the blazor app are in the publish folder

Actual Results

There are 3 .c files and 2 .h files in bin\Release\net6.0\publish\wwwroot\_framework:

  • corebindings.c
  • driver.c
  • pinvoke.c
  • pinvoke.h
  • pinvoke-table.h

image

These files shouldn't be published to the output folder.

cc @lewing @CoffeeFlux

Author: eerhardt
Assignees: -
Labels:

arch-wasm

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Setup untriaged New issue has not been triaged by the area owner labels Feb 8, 2021
@lewing lewing removed the untriaged New issue has not been triaged by the area owner label Feb 8, 2021
@lewing lewing added this to the 6.0.0 milestone Feb 8, 2021
@am11
Copy link
Member

am11 commented Feb 9, 2021

This seems to be intentional: #43785 (review). I think if we don't need the intermediate files, one cleaner option is that we separate out the "runtime pack with intermediates" from "standard runtime pack" (as the former is not for the common use-case for end-users, and requires WebAssembly compiler toolchain to be installed; to be able to relink with different arguments).

e.g. ./build.sh -os browser -c Release will not put these intermediate artifacts in artifacts//bin/native/net6.0-Browser-Release-wasm/src/, but, say a new OS, ./build.sh -os browser-extras -c Release will. This way, we will not need to modify src/installer/pkg/sfx/Microsoft.NETCore.App/Directory.Build.props, as that is a flat whitelist of 'all possible files' and sfx targets (in arcade) validate the other way around; all files in destination directory (in this case artifacts//bin/native/net6.0-Browser-Release-wasm/src/) are present in the whitelist. So if a subset (like clr or mono) doesn't produce all those files, it is fine.

cc @vargaz

BTW, here is the complete diff between net5 and net6 (6.0.0-preview.2.21105.12; 0429344) wwwroot/_framework directories:

--- net5
+++ net6
 System.Runtime.CompilerServices.Unsafe.dll
 System.Runtime.CompilerServices.Unsafe.dll.br
 System.Runtime.CompilerServices.Unsafe.dll.gz
-System.Runtime.InteropServices.RuntimeInformation.dll
-System.Runtime.InteropServices.RuntimeInformation.dll.br
-System.Runtime.InteropServices.RuntimeInformation.dll.gz
 System.Text.Encodings.Web.dll
 System.Text.Encodings.Web.dll.br
 System.Text.Encodings.Web.dll.gz
 System.Text.Json.dll
 System.Text.Json.dll.br
 System.Text.Json.dll.gz
+binding_support.js
+binding_support.js.br
+binding_support.js.gz
 blazor.boot.json
 blazor.boot.json.br
 blazor.boot.json.gz
@@ -106,15 +106,30 @@
 my-blazor-app.dll
 my-blazor-app.dll.br
 my-blazor-app.dll.gz
-dotnet.5.0.0.js
-dotnet.5.0.0.js.br
-dotnet.5.0.0.js.gz
+corebindings.c
+corebindings.c.br
+corebindings.c.gz
+dotnet.6.0.0-preview.2.21105.12.js
+dotnet.6.0.0-preview.2.21105.12.js.br
+dotnet.6.0.0-preview.2.21105.12.js.gz
 dotnet.timezones.blat
 dotnet.timezones.blat.br
 dotnet.timezones.blat.gz
 dotnet.wasm
 dotnet.wasm.br
 dotnet.wasm.gz
+dotnet_support.js
+dotnet_support.js.br
+dotnet_support.js.gz
+driver.c
+driver.c.br
+driver.c.gz
+emcc-flags.txt
+emcc-flags.txt.br
+emcc-flags.txt.gz
+emcc-version.txt
+emcc-version.txt.br
+emcc-version.txt.gz
 icudt.dat
 icudt.dat.br
 icudt.dat.gz
@@ -127,3 +142,18 @@
 icudt_no_CJK.dat
 icudt_no_CJK.dat.br
 icudt_no_CJK.dat.gz
+library_mono.js
+library_mono.js.br
+library_mono.js.gz
+pal_random.js
+pal_random.js.br
+pal_random.js.gz
+pinvoke-table.h
+pinvoke-table.h.br
+pinvoke-table.h.gz
+pinvoke.c
+pinvoke.c.br
+pinvoke.c.gz
+pinvoke.h
+pinvoke.h.br
+pinvoke.h.gz

@vargaz
Copy link
Contributor

vargaz commented Feb 9, 2021

We need those files to compile the final dotnet executable, at least driver.c needs to stay a source file since its compiled with -D defines during the build. Maybe we can get them from the runtime pack instead of the publish dir. Not sure how they end up in the final app, we create the final app ourself in the bin/Release/AppBundle directory which doesn't contain these files.

@am11
Copy link
Member

am11 commented Feb 11, 2021

Looks like in https://github.com/dotnet/sdk/blob/9ccce1ee9788b8267e0242f73b74bf9766bb20d0/src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets, due to line 95 and line 211, everything under ~/.nuget/packages/microsoft.netcore.app.runtime.browser-wasm/6.0.0-preview.2.21105.12/runtimes/browser-wasm/native gets copied to publish dir's wwwroot/_framework/. We can copy these additional *.c and *.h files somewhere else, say runtimes/browser-wasm/sources instead of runtimes/browser-wasm/native in this package without changing the SDK. Would that work?

Also, (unless it was intentional) the missing System.Runtime.InteropServices.RuntimeInformation.dll from .NET6 publish directory could be problematic.

@eerhardt
Copy link
Member Author

Also, (unless it was intentional) the missing System.Runtime.InteropServices.RuntimeInformation.dll from .NET6 publish directory could be problematic.

It isn't problematic. This assembly is no longer necessary. In net5.0, AspNetCore.Components was using the RuntimeInformation.IsOSPlatform method from this assembly. In net6.0, that code has been changed to use the new OperatingSystem.IsXXX() methods. See dotnet/aspnetcore#28233.

@pranavkm
Copy link
Contributor

By the way, we could change the SDK to have be more deliberate about what files are included (.dat, .blat, and the dotnet.wasm file). The SDK's globbing is very permissive today and there's no reason we couldn't flip it around

@eerhardt
Copy link
Member Author

The SDK's globbing is very permissive today and there's no reason we couldn't flip it around

I believe this is intentional with the reasoning being that the runtime gets to pick what is and what isn't included in the output of the application. If the file is in the right directory of the runtimepack, the intention is it is needed at runtime.

We had similar conversations about .a files and removed them in #41966.

@pranavkm
Copy link
Contributor

pranavkm commented Feb 15, 2021

We had similar conversations about .a files and removed them in #41966.

There's still a bunch of these in the runtime:

image

The SDK explicitly omits them:

Note: Still here in 6.0

image

@marek-safar
Copy link
Contributor

marek-safar commented Feb 15, 2021

The fact that the files are in the runtime pack does not mean they need to be in publish folder. Doing the work in BlazorWebAssembly is not the right place. It should be done by runtime tooling

@eerhardt
Copy link
Member Author

There's still a bunch of these in the runtime

It looks like #41966 was undone for $(TargetsMobile) == true

<ReferenceCopyLocalPaths Include="@(LibrariesRuntimeFiles)"
Condition="'%(LibrariesRuntimeFiles.Extension)' != '.a' or '$(TargetsMobile)' == 'true'">
<TargetPath Condition="'%(LibrariesRuntimeFiles.NativeSubDirectory)' != ''">runtimes/$(RuntimeIdentifier)/native/%(LibrariesRuntimeFiles.NativeSubDirectory)%(RecursiveDir)</TargetPath>

The runtimes/<RID>/native folder is not the correct place for static library .a files, .c, or .h files. This folder is for architecture-specific files that are needed at runtime. That is why the SDK is publishing them to the output folder by default. The design is: If a file is contained in the runtimes/<RID>/native folder, it is needed at runtime so publish it to the output folder.

@lewing
Copy link
Member

lewing commented Feb 18, 2021

so what is the correct place for architecture-specific files needed to create the publishable runtime?

@marek-safar
Copy link
Contributor

Some of these are temporary build files, we probably need some msbuild magic to store them in temporary obj/ folder

@am11
Copy link
Member

am11 commented Feb 18, 2021

We probably just need a property next to:

<MicrosoftNetCoreAppRuntimePackNativeDir>$([MSBuild]::NormalizeDirectory('$(MicrosoftNetCoreAppRuntimePackRidDir)', 'native'))</MicrosoftNetCoreAppRuntimePackNativeDir>
say MicrosoftNetCoreAppRuntimePackSourceDir: $([MSBuild]::NormalizeDirectory('$(MicrosoftNetCoreAppRuntimePackRidDir)', 'source') and then adjust this item group:
<ItemGroup>
<IcuDataFiles Include="$(NativeBinDir)*.dat" />
<WasmSrcFiles Include="$(NativeBinDir)src\*.c;
$(NativeBinDir)src\*.js;
$(NativeBinDir)src\emcc-flags.txt;
$(NativeBinDir)src\emcc-version.txt" />
<WasmHeaderFiles Include="$(NativeBinDir)include\wasm\*.h" />
</ItemGroup>
<Copy SourceFiles="$(NativeBinDir)dotnet.js;
$(NativeBinDir)dotnet.wasm;
$(NativeBinDir)dotnet.timezones.blat;
$(NativeBinDir)libicuuc.a;
$(NativeBinDir)libicui18n.a"
DestinationFolder="$(MicrosoftNetCoreAppRuntimePackNativeDir)"
SkipUnchangedFiles="true" />
<Copy SourceFiles="@(IcuDataFiles)"
DestinationFolder="$(MicrosoftNetCoreAppRuntimePackNativeDir)"
SkipUnchangedFiles="true" />
<Copy SourceFiles="@(WasmSrcFiles)"
DestinationFolder="$(MicrosoftNetCoreAppRuntimePackNativeDir)src"
SkipUnchangedFiles="true" />
<Copy SourceFiles="@(WasmHeaderFiles)"
DestinationFolder="$(MicrosoftNetCoreAppRuntimePackNativeDir)include\wasm"
SkipUnchangedFiles="true" />
</Target>
the (niche) consumers of driver.c and friends will look for {package-path}/source/ instead of {package-path}/native/.

@eerhardt
Copy link
Member Author

so what is the correct place for architecture-specific files needed to create the publishable runtime?

I believe we will need to "make one up", like @am11 is suggesting above. I don't believe there is already a "canonical" place for these kinds of assets.

Looking deeper into how this works, another possibility would be to remove these files from the RuntimeList.xml file. The ResolveRuntimePackAssets looks there for which files to copy out of the runtimepack:

https://github.com/dotnet/sdk/blob/b91b88aec2684e3d2988df8d838d3aa3c6240a35/src/Tasks/Microsoft.NET.Build.Tasks/ResolveRuntimePackAssets.cs#L82-L105

I believe @dsplaisted or @ericstj should be able to give guidance here on what the correct approach is for files that ship in the runtimepack, but aren't supposed to be published to the end user's publish folder.

@radical
Copy link
Member

radical commented Feb 18, 2021

Some of these are temporary build files, we probably need some msbuild magic to store them in temporary obj/ folder

that should already be fixed on master

@dsplaisted
Copy link
Member

I think that it's correct that you can just remove the files from RuntimeList.xml if you don't want them published. If other tooling needs those files, then it will need to find them somehow. You could do it by convention, looking in the same folder as some runtime asset, or in a different folder relative to that folder if that's cleaner.

We also might want to update the RuntimeList.xml format so it can express different types of assets. Probably in that case we'd update the ResolveRuntimePackAssets task output the different assets in different MSBuild items.

@eerhardt
Copy link
Member Author

FYI - this was also logged in ASP.NET - dotnet/aspnetcore#32744.

@radical
Copy link
Member

radical commented Sep 9, 2021

This is not an issue for blazorwasm projects anymore, as it is skipping those files. So, moving this to 7.0.0, to fix for non-blazorwasm projects.

@radical radical modified the milestones: 6.0.0, 7.0.0 Sep 9, 2021
@radical radical changed the title Some .c and .h files are being published for Blazor WASM apps Some .c and .h files are being published for non-blazorwasm apps Sep 9, 2021
@lewing
Copy link
Member

lewing commented May 12, 2022

We need to decide on a resolution to this so that the various bundlers can share it.

@radical
Copy link
Member

radical commented Aug 10, 2022

This is being handled currently, but in a hacky manner. And we have an approach in mind, but that will get implemented in .net8 .

@radical radical modified the milestones: 7.0.0, 8.0.0 Aug 10, 2022
@maraf
Copy link
Member

maraf commented Jul 24, 2023

This is resolved in Wasm SDK

@maraf maraf closed this as completed Jul 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

No branches or pull requests

9 participants