Added wasi-sdk flow #307

mkhamoyan · 2023-01-17T14:32:51Z

Contributes to dotnet/runtime#65895

Allows to build icu for wasm using wasi-sdk.

eng/icu.mk

eng/icu.wasi.mk

steveisok

LGTM! Have you been able to use it yet in the wasi runtime build?

mkhamoyan · 2023-01-23T07:26:36Z

LGTM! Have you been able to use it yet in the wasi runtime build?

not yet. Once this will be merged I will do changes in wasi runtime build

pavelsavara

the changes which are also in patch need to be removed from the PR, right ?

mkhamoyan · 2023-01-23T11:51:32Z

the changes which are also in patch need to be removed from the PR, right ?

I checked with other patches and they were applied to the code that is why I didn't remove from PR. Also if I remove these changes , I didn't find code which will apply the patch before running on CI. Maybe @directhex or @akoeplinger will know?

radical · 2023-01-23T19:13:19Z

eng/azure-pipelines.yml

+          inputs:
+            targetPath: 'artifacts/bin'
+            artifactName: Artifacts_Wasi_Threading
+            condition: and(succeeded(), ne(variables['System.TeamProject'], 'public'), ne(variables['Build.Reason'], 'PullRequest'))      


Suggested change

condition: and(succeeded(), ne(variables['System.TeamProject'], 'public'), ne(variables['Build.Reason'], 'PullRequest'))

condition: and(succeeded(), ne(variables['System.TeamProject'], 'public'), ne(variables['Build.Reason'], 'PullRequest'))

eng/icu.proj

radical · 2023-01-23T19:18:34Z

eng/icu.proj

+    </ReadLinesFromFile>
+
+    <PropertyGroup>
+      <WASI_SDK_PATH>$(RepoRoot)\artifacts</WASI_SDK_PATH>


Is this needed here? What is using it?

Removed, was not used.

radical · 2023-01-23T19:19:55Z

eng/icu.proj

+      <WASI_SDK_PATH>$(RepoRoot)\artifacts</WASI_SDK_PATH>
+      <WasiLocalPath>$(RepoRoot)\artifacts</WasiLocalPath>
+      <WasiSdkVersion>%(_VersionLines.Identity)</WasiSdkVersion>
+      <WasiSdkUrl>https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-$(WasiSdkVersion)/wasi-sdk-$(WasiSdkVersion).0-linux.tar.gz</WasiSdkUrl>


Condition for OS here too. And we can use <Error if the url is not set, eg. on windows.

radical · 2023-01-23T19:20:51Z

eng/icu.proj

+      <WasiLocalPath>$(RepoRoot)\artifacts</WasiLocalPath>
+      <WasiSdkVersion>%(_VersionLines.Identity)</WasiSdkVersion>
+      <WasiSdkUrl>https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-$(WasiSdkVersion)/wasi-sdk-$(WasiSdkVersion).0-linux.tar.gz</WasiSdkUrl>


These properties are only for private use, so prepend _ to the names like _WasiLocalPath.

radical · 2023-01-23T19:26:45Z

eng/icu.proj

+      <WasiSdkUrl Condition="$([MSBuild]::IsOSPlatform('OSX'))" >https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-$(WasiSdkVersion)/wasi-sdk-$(WasiSdkVersion).0-macos.tar.gz</WasiSdkUrl>
+    </PropertyGroup>
+
+    <Exec Command="rm -rf wasi-sdk-*/ wasi-sdk/ &amp;&amp; curl -L -o- $(WasiSdkUrl) | tar -xvz &amp;&amp; mv wasi-sdk-$(WasiSdkVersion).*/ wasi-sdk/"


This won't show any errors with downloading the tarball. Add --fail -S to the curl command.

I guess we could do the same improvements on runtime repo

radical · 2023-01-23T19:31:18Z

eng/icu.wasi.mk

+        CFLAGS="-Oz -fno-exceptions -Wno-sign-compare $(THREADS_FLAG) $(ICU_DEFINES)" \
+        CXXFLAGS="-Oz -fno-exceptions -Wno-sign-compare $(THREADS_FLAG) $(ICU_DEFINES)" \
+        CC="$(WASI_SDK_PATH)/bin/clang --sysroot=$(WASI_SDK_PATH)/share/wasi-sysroot" \
+		CXX="$(WASI_SDK_PATH)/bin/clang++ --sysroot=$(WASI_SDK_PATH)/share/wasi-sysroot" \
+		--host=$(HOST_PLATFORM) --build=wasm32


spaces -> tabs for make file.

Suggested change

CFLAGS="-Oz -fno-exceptions -Wno-sign-compare $(THREADS_FLAG) $(ICU_DEFINES)" \

CXXFLAGS="-Oz -fno-exceptions -Wno-sign-compare $(THREADS_FLAG) $(ICU_DEFINES)" \

CC="$(WASI_SDK_PATH)/bin/clang --sysroot=$(WASI_SDK_PATH)/share/wasi-sysroot" \

CXX="$(WASI_SDK_PATH)/bin/clang++ --sysroot=$(WASI_SDK_PATH)/share/wasi-sysroot" \

--host=$(HOST_PLATFORM) --build=wasm32

CFLAGS="-Oz -fno-exceptions -Wno-sign-compare $(THREADS_FLAG) $(ICU_DEFINES)" \

CXXFLAGS="-Oz -fno-exceptions -Wno-sign-compare $(THREADS_FLAG) $(ICU_DEFINES)" \

CC="$(WASI_SDK_PATH)/bin/clang --sysroot=$(WASI_SDK_PATH)/share/wasi-sysroot" \

CXX="$(WASI_SDK_PATH)/bin/clang++ --sysroot=$(WASI_SDK_PATH)/share/wasi-sysroot" \

--host=$(HOST_PLATFORM) --build=wasm32

radical · 2023-01-23T19:31:33Z

eng/icu.wasi.mk

+		--host=$(HOST_PLATFORM) --build=wasm32
+
+check-env:
+  :


space -> tab

Suggested change

:

:

radical · 2023-01-23T19:40:17Z

eng/azure-pipelines.yml

@@ -188,6 +235,14 @@ stages:
          inputs:
            artifact: Artifacts_Browser_Threading
            path: 'artifacts/bin/browser-threading'
+        - task: DownloadPipelineArtifact@2


For a future PR: This Publish Packages job should run for PRs too, and generate the packages, but skip the publishing part itself. This would ensure that the PR doesn't break packaging.

pavelsavara · 2023-01-24T11:51:29Z

the changes which are also in patch need to be removed from the PR, right ?

I checked with other patches and they were applied to the code that is why I didn't remove from PR. Also if I remove these changes , I didn't find code which will apply the patch before running on CI. Maybe @directhex or @akoeplinger will know?

Does anyone know why we have both the patch files and the changes in files ?

akoeplinger · 2023-01-24T14:04:14Z

The patch files in icu-patches/ are from the upstream Microsoft fork of the ICU repo, not ours in dotnet. We haven't been keeping patch files for our changes, but just committing directly (with the disadvantage of having to go back and reapply changes when we move to a new ICU branch).

got explanation already

pavelsavara · 2023-01-25T08:17:46Z

eng/icu.proj

@@ -37,4 +39,18 @@
            Lines="@(_ItemsFromEmscriptenVersionFile->Replace('&quot;', ''))"
            Overwrite="true" />
  </Target>
+  <Target Name="PatchWasiSdk" Condition="'$(TargetOS)' == 'Wasi'">


Please rename to DownloadWasiSdk. Also I'm not clear if this is necessary when running on docker image with wasi SDK already in place.

Updated, now it will download wasi sdk only when needed.

Added wasi-sdk flow

ad1ca5e

mkhamoyan requested review from akoeplinger, pavelsavara, radical and steveisok January 17, 2023 14:39

pavelsavara reviewed Jan 17, 2023

View reviewed changes

eng/icu.mk Show resolved Hide resolved

mkhamoyan added 8 commits January 17, 2023 16:20

Added wasi in azure-pipelines

426b8ff

Removed container name

2525da0

Change Wasi_SDK_PATH for CI

a0cb465

update paths

17beb4d

update path

3f2da6c

update path

f38f521

update path

915bc84

change wasi-sdk-version

90c071d

steveisok reviewed Jan 17, 2023

View reviewed changes

eng/icu.mk Show resolved Hide resolved

mkhamoyan added 7 commits January 18, 2023 08:57

Updated PatchWasiSdk

b1ed769

update icu.proj

d5fd0ef

update icu.proj

a4b8734

update the root

a536b93

update icu.proj

bf0e0aa

upate icu.proj

a443d2c

update icu.proj

3f15161

pavelsavara reviewed Jan 18, 2023

View reviewed changes

eng/icu.wasi.mk Show resolved Hide resolved

mkhamoyan requested a review from directhex January 18, 2023 12:48

mkhamoyan added 6 commits January 18, 2023 15:08

update wasi clang paths

2b441a6

update comfiler-flags

a8e96b4

update compiler_flags

32e532b

update compiler_flags

f244f8b

update compiler flags

4ab7653

update compiler config

d717f61

mkhamoyan added 4 commits January 20, 2023 08:43

Added patch for icu changes

a7d2c77

update hardcoded HOST_PLATFORM

40a1e89

Remove not used host

605ef11

update condition for platform check

1163a29

mkhamoyan requested review from pavelsavara and steveisok January 20, 2023 12:25

mkhamoyan marked this pull request as ready for review January 20, 2023 12:27

steveisok approved these changes Jan 20, 2023

View reviewed changes

pavelsavara previously requested changes Jan 23, 2023

View reviewed changes

radical reviewed Jan 23, 2023

View reviewed changes

Merge branch 'dotnet/main' into icu_wasi_sdk

1b926d9

Refactoring

ea55666

mkhamoyan requested a review from radical January 24, 2023 14:52

This comment was marked as outdated.

Sign in to view

pavelsavara approved these changes Jan 25, 2023

View reviewed changes

mkhamoyan added 4 commits January 25, 2023 11:18

download wasi-sdk when needed

17e2613

Added comments

30a53d9

Remove unused code

3036bc2

Removed extra line

ca26058

mkhamoyan enabled auto-merge (squash) January 26, 2023 07:31

mkhamoyan disabled auto-merge January 26, 2023 07:35

update

e435609

mkhamoyan merged commit 4d40948 into dotnet:dotnet/main Jan 26, 2023

This was referenced Jan 27, 2023

Add wasi-wasm in icu package #309

Merged

WASI support tracking dotnet/runtime#65895

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added wasi-sdk flow #307

Added wasi-sdk flow #307

mkhamoyan commented Jan 17, 2023 •

edited

Loading

steveisok left a comment

mkhamoyan commented Jan 23, 2023

pavelsavara left a comment

mkhamoyan commented Jan 23, 2023

radical Jan 23, 2023

radical Jan 23, 2023

mkhamoyan Jan 24, 2023

radical Jan 23, 2023

radical Jan 23, 2023

radical Jan 23, 2023

pavelsavara Jan 24, 2023

radical Jan 23, 2023

radical Jan 23, 2023

radical Jan 23, 2023

pavelsavara commented Jan 24, 2023

akoeplinger commented Jan 24, 2023

This comment was marked as outdated.

pavelsavara Jan 25, 2023

mkhamoyan Jan 26, 2023

	condition: and(succeeded(), ne(variables['System.TeamProject'], 'public'), ne(variables['Build.Reason'], 'PullRequest'))
	condition: and(succeeded(), ne(variables['System.TeamProject'], 'public'), ne(variables['Build.Reason'], 'PullRequest'))

Added wasi-sdk flow #307

Added wasi-sdk flow #307

Conversation

mkhamoyan commented Jan 17, 2023 • edited Loading

steveisok left a comment

Choose a reason for hiding this comment

mkhamoyan commented Jan 23, 2023

pavelsavara left a comment

Choose a reason for hiding this comment

mkhamoyan commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelsavara commented Jan 24, 2023

akoeplinger commented Jan 24, 2023

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Jan 17, 2023 •

edited

Loading