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

[mono][wasm] Include NativeLibrary items to the NativeFileReference items #101532

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Apr 25, 2024

Fixes #96864

@mkhamoyan mkhamoyan self-assigned this Apr 25, 2024
@mkhamoyan mkhamoyan added arch-wasm WebAssembly architecture os-wasi Related to WASI variant of arch-wasm os-browser Browser variant of arch-wasm labels Apr 25, 2024
src/mono/browser/build/BrowserWasmApp.targets Outdated Show resolved Hide resolved
src/mono/wasi/build/WasiApp.targets Outdated Show resolved Hide resolved
{
string projectName = $"AppUsingNativeLibrary-a";
buildArgs = buildArgs with { ProjectName = projectName };
buildArgs = ExpandBuildArgs(buildArgs, extraItems: "<NativeLibrary Include=\"native-lib.o\" />");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buildArgs = ExpandBuildArgs(buildArgs, extraItems: "<NativeLibrary Include=\"native-lib.o\" />");
buildArgs = ExpandBuildArgs(buildArgs, extraItems: "<NativeLibrary Include=\"native-lib.o\" />\n<NativeLibrary Include=\"DoesNotExist.o\" \>");

Copy link
Member

Choose a reason for hiding this comment

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

This will fail the build, right? It would be ideal to have it conditional as theory data

Copy link
Member

Choose a reason for hiding this comment

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

We need to make it not fail the build since NativeLibrary does not mind if the file doesn't exist

src/mono/wasm/build/WasmApp.Common.targets Outdated Show resolved Hide resolved
{
string projectName = $"AppUsingNativeLibrary-a";
buildArgs = buildArgs with { ProjectName = projectName };
buildArgs = ExpandBuildArgs(buildArgs, extraItems: "<NativeLibrary Include=\"native-lib.o\" />");
Copy link
Member

Choose a reason for hiding this comment

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

This will fail the build, right? It would be ideal to have it conditional as theory data

@@ -249,6 +249,7 @@
</PropertyGroup>

<ItemGroup>
<NativeFileReference Include="@(NativeLibrary->'%(Identity)')" Condition="'@(NativeLibrary->Count())' != '0'" />
Copy link
Member

@lewing lewing Apr 25, 2024

Choose a reason for hiding this comment

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

Suggested change
<NativeFileReference Include="@(NativeLibrary->'%(Identity)')" Condition="'@(NativeLibrary->Count())' != '0'" />
<_ExistingNativeLibrary Include="@(NativeLibrary->Exists())" />
<NativeFileReference Include="@(_ExistingNativeLibrary->'%(Identity)')" />

We need something that does effectively this (or a cleaner expression of this)

Copy link
Member

@maraf maraf Apr 26, 2024

Choose a reason for hiding this comment

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

Would it make sense to move the condition and items copying into some target executed just before we need these items? So that the lib can be created or copied by some user's target?

EDIT: Hmm. That would a change in evaluation if we need native build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved under _InitializeCommonProperties

<Target Name="_InitializeCommonProperties">

With this it works when NativeLibrary is added under user's target.

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

For the sake of compatibility we should probably ignore missing NativeLibrary entries.

@mkhamoyan mkhamoyan requested a review from lewing April 26, 2024 08:29
@lewing lewing merged commit cab2d11 into dotnet:main Apr 26, 2024
30 of 35 checks passed
@mkhamoyan mkhamoyan deleted the include_native_library branch April 29, 2024 13:06
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…tems (dotnet#101532)

* Include NativeLibary and add test case

* Move to common place

* Ignore missing NativeLibraries

* Move items copying into target
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…tems (dotnet#101532)

* Include NativeLibary and add test case

* Move to common place

* Ignore missing NativeLibraries

* Move items copying into target
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…tems (dotnet#101532)

* Include NativeLibary and add test case

* Move to common place

* Ignore missing NativeLibraries

* Move items copying into target
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2024
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 os-browser Browser variant of arch-wasm os-wasi Related to WASI variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] NativeLibrary doesn't include file
3 participants