-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[WASI] bump WASI SDK to v25.0 #110654
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
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
| <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.
radekdoulik
left a comment
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
| <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.
jsturtevant
left a comment
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.0as version detection sanity file inside runtime repo$(WASI_SDK_PATH)/VERSIONin workload outside of runtime repo, which is part of the release .tarD_WASI_EMULATED_PTHREADfor wasi-sdk, which is an empty implementationwasm-optdetectionFixes #104773
Related dotnet/dotnet-buildtools-prereqs-docker#1299