-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[WASI] bump WASI SDK to v25.0 #110654
[WASI] bump WASI SDK to v25.0 #110654
Conversation
- use `WASI-SDK-VERSION-25.0` as version detection sanity file inside runtime repo - read file $(WASI_SDK_PATH)/VERSION in workload outside of runtime repo
@@ -1,6 +1,6 @@ | |||
<Project> | |||
<PropertyGroup> | |||
<WASI_SDK_PATH Condition="'$(WASI_SDK_PATH)' == '' or !Exists('$(WASI_SDK_PATH)/VERSION24')">$([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), '..', 'wasi-sdk'))</WASI_SDK_PATH> | |||
<WASI_SDK_PATH Condition="'$(WASI_SDK_PATH)' == '' or !Exists('$(WASI_SDK_PATH)/VERSION')">$([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), '..', 'wasi-sdk'))</WASI_SDK_PATH> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be WASI-SDK-VERSION-25.0
instead of VERSION
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the VERSION
file that wasi-sdk ships with. I don't know why we also write WASI-SDK-VERSION-25.0
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, VERSION
is in the sdk .tar file.
We touch WASI-SDK-VERSION-25.0
only do that inside of the runtime repo.
We do it because it's easy way how to sanity-check version of the WASI-SDK in various places of MSBuild/CI pipeline.
Because it's one-liner Condition
, instead of <ReadLinesFromFile ...
in a Target
Does that make sense ?
Obviously it could be refactored, into Target
in some common place. I don't know where to ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do something better for user machines.
There isn't a shared MSBuild file for runtime & user machine build I know of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The users machine check is the actual ReadLinesFromFile
, no need to create dummy file there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean better than this, let's improve that in next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside the comment it LGTM
@@ -1,6 +1,6 @@ | |||
<Project> | |||
<PropertyGroup> | |||
<WASI_SDK_PATH Condition="'$(WASI_SDK_PATH)' == '' or !Exists('$(WASI_SDK_PATH)/VERSION24')">$([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), '..', 'wasi-sdk'))</WASI_SDK_PATH> | |||
<WASI_SDK_PATH Condition="'$(WASI_SDK_PATH)' == '' or !Exists('$(WASI_SDK_PATH)/VERSION')">$([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), '..', 'wasi-sdk'))</WASI_SDK_PATH> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do something better for user machines.
There isn't a shared MSBuild file for runtime & user machine build I know of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
WASI-SDK-VERSION-25.0
as version detection sanity file inside runtime repo$(WASI_SDK_PATH)/VERSION
in workload outside of runtime repo, which is part of the release .tarD_WASI_EMULATED_PTHREAD
for wasi-sdk, which is an empty implementationwasm-opt
detectionFixes #104773
Related dotnet/dotnet-buildtools-prereqs-docker#1299