-
-
Notifications
You must be signed in to change notification settings - Fork 983
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
Make wasm-interpreter work like wasm-aot #1769
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,7 +85,6 @@ protected void GenerateProjectInterpreter(BuildPartition buildPartition, Artifac | |
.Replace("$SDKNAME$", sdkName) | ||
.Replace("$RUNTIMEPACK$", CustomRuntimePack ?? "") | ||
.Replace("$TARGET$", CustomRuntimePack != null ? "PublishWithCustomRuntimePack" : "Publish") | ||
.Replace("$MAINJS$", runtime.MainJs.ToString()) | ||
.ToString(); | ||
|
||
File.WriteAllText(artifactsPaths.ProjectFilePath, content); | ||
|
@@ -96,14 +95,7 @@ protected void GenerateProjectInterpreter(BuildPartition buildPartition, Artifac | |
|
||
protected override string GetBinariesDirectoryPath(string buildArtifactsDirectoryPath, string configuration) | ||
{ | ||
if (Aot) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this change and I would like to be sure that we are not breaking anything. Have you tested both AOT and non-AOT toolchains? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, before wasm-intepreter mode was using a simplified custom app builder I built into BDN, because that was the best thing that could be done at the time. Wasm-aot used all the app bundling stuff from the the runtime source directory, which put things in a different place than the one I coded into BDN. With this change, both wasm-interpreter and wasm-aot will use the app builder from the runtime source dir, so the files will end up in the same place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tested both AOT and interpreter configurations. |
||
{ | ||
return Path.Combine(buildArtifactsDirectoryPath, "bin", TargetFrameworkMoniker, "browser-wasm", "AppBundle"); | ||
} | ||
else | ||
{ | ||
return Path.Combine(buildArtifactsDirectoryPath, "bin", TargetFrameworkMoniker, "browser-wasm", "publish", "output"); | ||
} | ||
} | ||
} | ||
} |
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.
Please keep in mind that once we delete this parameter and you update BDN in dotnet/performance everything that is still using this argument is going to get an error from the BDN config parser
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.
Yes, I am aware. We have made changes in dotnet/performance so nothing is using it.