Skip to content

[wasm] Disallow not useful configuration combinations #104149

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jul 4, 2024
Merged

[wasm] Disallow not useful configuration combinations #104149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
87e7399
Fix - aot doesn't support managed debugging
ilonatommy Jun 28, 2024
3a2c963
Fix aot-profiling sample.
ilonatommy Jul 1, 2024
6c3ef4c
Fix browser wbt
ilonatommy Jul 1, 2024
a60e430
Fix wasi wbt.
ilonatommy Jul 2, 2024
c87e7da
fix wbt
ilonatommy Jul 2, 2024
4fc8e2a
Fix
ilonatommy Jul 2, 2024
afb6e0d
Debug + aot on linux should not be tested in this test.
ilonatommy Jul 2, 2024
ca1b119
Cleanup of previous commit.
ilonatommy Jul 2, 2024
7451fde
Revert changes to cwraps, fix the sample.
ilonatommy Jul 2, 2024
35aedcd
Feedback
ilonatommy Jul 3, 2024
90132be
Remove aot + debug.
ilonatommy Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/mono/browser/runtime/cwraps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ const fn_signatures: SigLine[] = [
[true, "mono_wasm_set_main_args", "void", ["number", "number"]],
// These two need to be lazy because they may be missing
[() => !runtimeHelpers.emscriptenBuildOptions.enableAotProfiler, "mono_wasm_profiler_init_aot", "void", ["string"]],
[() => !runtimeHelpers.emscriptenBuildOptions.enableBrowserProfiler, "mono_wasm_profiler_init_aot", "void", ["string"]],
[() => !runtimeHelpers.emscriptenBuildOptions.enableBrowserProfiler, "mono_wasm_profiler_init_browser", "void", ["string"]],
[true, "mono_wasm_profiler_init_browser", "void", ["number"]],
[false, "mono_wasm_exec_regression", "number", ["number", "string"]],
[false, "mono_wasm_invoke_jsexport", "void", ["number", "number"]],
Expand Down
3 changes: 2 additions & 1 deletion src/mono/wasi/Wasi.Build.Tests/WasiTemplateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ public void ConsoleBuildThenRunThenPublish(string config, bool singleFileBundle,
CreateProject: false,
Publish: true,
TargetFramework: BuildTestBase.DefaultTargetFramework,
UseCache: false));
UseCache: false,
ExpectSuccess: !(config == "Debug" && aot)));
}

[Theory]
Expand Down
27 changes: 22 additions & 5 deletions src/mono/wasm/Wasm.Build.Tests/Blazor/BuildPublishTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,20 @@ public async Task DefaultTemplate_WithoutWorkload(string config)
public static TheoryData<string, bool> TestDataForDefaultTemplate_WithWorkload(bool isAot)
{
var data = new TheoryData<string, bool>();
data.Add("Debug", false);
data.Add("Release", false); // Release relinks by default
// [ActiveIssue("https://github.com/dotnet/runtime/issues/83497", TestPlatforms.Windows)]
if (!isAot || !RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
if (!isAot)
{
data.Add("Debug", true); // for aot:true on Windows, it fails
// AOT does not support managed debugging, is disabled by design
data.Add("Debug", false);
}
data.Add("Release", false); // Release relinks by default

// [ActiveIssue("https://github.com/dotnet/runtime/issues/83497", TestPlatforms.Windows)]
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
if (!isAot)
{
data.Add("Debug", true);
}
data.Add("Release", true);
}
return data;
Expand Down Expand Up @@ -197,4 +200,18 @@ public async Task Test_WasmStripILAfterAOT(string stripILAfterAOT, bool expectIL

WasmTemplateTests.TestWasmStripILAfterAOTOutput(objBuildDir, frameworkDir, expectILStripping, _testOutput);
}

[Theory]
[InlineData("Debug")]
public void BlazorWasm_CannotAOT_InDebug(string config)
{
string id = $"blazorwasm_{config}_aot_{GetRandomId()}";
CreateBlazorWasmTemplateProject(id);
AddItemsPropertiesToProject(Path.Combine(_projectDir!, $"{id}.csproj"),
extraItems: null,
extraProperties: "<RunAOTCompilation>true</RunAOTCompilation>");

(CommandResult res, _) = BlazorPublish(new BlazorBuildOptions(id, config, ExpectSuccess: false));
Assert.Contains("AOT is not supported in debug configuration", res.Output);
}
}
1 change: 0 additions & 1 deletion src/mono/wasm/Wasm.Build.Tests/Blazor/MiscTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ public void NativeBuild_WithDeployOnBuild_UsedByVS(string config, bool nativeRel
}

[Theory]
[InlineData("Debug")]
[InlineData("Release")]
public void DefaultTemplate_AOT_InProjectFile(string config)
{
Expand Down
1 change: 0 additions & 1 deletion src/mono/wasm/Wasm.Build.Tests/Blazor/SimpleRunTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ public async Task BlazorBuildAndRunForDifferentOutputPaths(string config, bool a

[Theory]
[InlineData("Debug", false)]
[InlineData("Debug", true)]
[InlineData("Release", false)]
[InlineData("Release", true)]
public async Task BlazorPublishRunTest(string config, bool aot)
Expand Down
23 changes: 22 additions & 1 deletion src/mono/wasm/Wasm.Build.Tests/BuildPublishTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,28 @@ public BuildPublishTests(ITestOutputHelper output, SharedBuildPerTestClassFixtur
{
}

[Theory]
[BuildAndRun(host: RunHost.Chrome, aot: true, config: "Debug")]
public void Wasm_CannotAOT_InDebug(BuildArgs buildArgs, RunHost _, string id)
{
string projectName = GetTestProjectPath(prefix: "no_aot_in_debug", config: buildArgs.Config);
buildArgs = buildArgs with { ProjectName = projectName };
buildArgs = ExpandBuildArgs(buildArgs);
(string projectDir, string buildOutput) = BuildProject(buildArgs,
id: id,
new BuildProjectOptions(
InitProject: () => File.WriteAllText(Path.Combine(_projectDir!, "Program.cs"), s_mainReturns42),
DotnetWasmFromRuntimePack: true,
CreateProject: true,
Publish: true,
ExpectSuccess: false
));

Console.WriteLine($"buildOutput={buildOutput}");

Assert.Contains("AOT is not supported in debug configuration", buildOutput);
}

[Theory]
[BuildAndRun(host: RunHost.Chrome, aot: false, config: "Release")]
[BuildAndRun(host: RunHost.Chrome, aot: false, config: "Debug")]
Expand Down Expand Up @@ -71,7 +93,6 @@ void Run() => RunAndTestWasmApp(

[Theory]
[BuildAndRun(host: RunHost.Chrome, aot: true, config: "Release")]
[BuildAndRun(host: RunHost.Chrome, aot: true, config: "Debug")]
public void BuildThenPublishWithAOT(BuildArgs buildArgs, RunHost host, string id)
{
bool testUnicode = true;
Expand Down
13 changes: 6 additions & 7 deletions src/mono/wasm/Wasm.Build.Tests/Templates/WasmTemplateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public WasmTemplateTests(ITestOutputHelper output, SharedBuildPerTestClassFixtur
{
}

private string StringReplaceWithAssert(string oldContent, string oldValue, string newValue)
private string StringReplaceWithAssert(string oldContent, string oldValue, string newValue)
{
string newContent = oldContent.Replace(oldValue, newValue);
if (oldValue != newValue && oldContent == newContent)
Expand Down Expand Up @@ -57,11 +57,11 @@ private void UpdateConsoleProgramCs()
private void UpdateBrowserMainJs(string targetFramework, string runtimeAssetsRelativePath = DefaultRuntimeAssetsRelativePath)
{
base.UpdateBrowserMainJs(
(mainJsContent) =>
(mainJsContent) =>
{
// .withExitOnUnhandledError() is available only only >net7.0
mainJsContent = StringReplaceWithAssert(
mainJsContent,
mainJsContent,
".create()",
(targetFramework == "net8.0" || targetFramework == "net9.0")
? ".withConsoleForwarding().withElementOnExit().withExitCodeLogging().withExitOnUnhandledError().create()"
Expand All @@ -75,8 +75,8 @@ private void UpdateBrowserMainJs(string targetFramework, string runtimeAssetsRel
mainJsContent = StringReplaceWithAssert(mainJsContent, "from './_framework/dotnet.js'", $"from '{runtimeAssetsRelativePath}dotnet.js'");

return mainJsContent;
},
targetFramework,
},
targetFramework,
runtimeAssetsRelativePath
);
}
Expand Down Expand Up @@ -121,7 +121,7 @@ public void BrowserBuildThenPublish(string config)

var buildArgs = new BuildArgs(projectName, config, false, id, null);

AddItemsPropertiesToProject(projectFile,
AddItemsPropertiesToProject(projectFile,
atTheEnd:
"""
<Target Name="CheckLinkedFiles" AfterTargets="ILLink">
Expand Down Expand Up @@ -389,7 +389,6 @@ public static TheoryData<string, bool, bool> TestDataForConsolePublishAndRun()
// [ActiveIssue("https://github.com/dotnet/runtime/issues/71887", TestPlatforms.Windows)]
if (!OperatingSystem.IsWindows())
{
data.Add("Debug", true, false);
data.Add("Release", true, false);
}

Expand Down
4 changes: 3 additions & 1 deletion src/mono/wasm/build/WasmApp.Common.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@
<ItemGroup>
<UpToDateCheckInput Include="@(NativeFileReference)" />
</ItemGroup>

<PropertyGroup>
<_WasmDebuggerSupport Condition="'$(WasmDebugLevel)' != '' and '$(WasmDebugLevel)' != '0'">true</_WasmDebuggerSupport>
<_WasmDebuggerSupport Condition="'$(WasmDebugLevel)' != '' and '$(WasmDebugLevel)' == '0'">false</_WasmDebuggerSupport>
Expand Down Expand Up @@ -621,6 +621,8 @@

<Error Condition="'$(RunAOTCompilation)' == 'true' and '$(PublishTrimmed)' != 'true'"
Text="AOT is not supported without IL trimming (PublishTrimmed=true required)." />
<Error Condition="'$(RunAOTCompilation)' == 'true' and '$(Configuration)' == 'Debug'"
Copy link
Member

Choose a reason for hiding this comment

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

What is the true underlaying requirement for AOT? The configuration name "Debug" is just a name, user can have a custom name.

Copy link
Member Author

@ilonatommy ilonatommy Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We rely on Configuration variable value, comparing it with Release and Debug in multiple places.

runtime/src/mono/wasm/build/WasmApp.Common.targets

Line 554 in 27d1ab0

<PropertyGroup Condition="'$(WasmNativeDebugSymbols)' == '' and '$(WasmNativeStrip)' == '' and '$(WasmBuildingForNestedPublish)' != 'true' and '$(WasmBuildNative)' == 'true' and '$(Configuration)' == 'Debug'">

runtime/src/mono/wasm/build/WasmApp.Common.targets

Line 546 in 27d1ab0

<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(Configuration)' == 'Release'">true</WasmBuildNative>

runtime/src/mono/mono.proj

Line 495 in 27d1ab0

<_MonoCPPFLAGS Condition="'$(Configuration)' == 'Debug'" Include="-D_DEBUG" />

Do you mean that there's a better property to check if we have debug symbols available?

Copy link
Member

@maraf maraf Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm aware of other places (that they exist) and every now and then they fail for some user with custom config name (there not much of them, but such users exist). So I would like to know if there is a better property to check and hence the question what is the true underlaying requirement for AOT compiler, so we could check that requirement instead.

If the requirement is one of the places where we check for config==debug, there is nothing to do about it. If the requirement is something different with some concrete property value, it would be great to check that property.

Copy link
Member Author

@ilonatommy ilonatommy Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see. The requirement: runtime without optimization (so debug-friendly) will take long time to build app with AOT and in the end it won't support managed debugging, so there was no point. Useful properties would be connected with managed debugging or optimization on/off (looking for such...).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not able to find a good alternative

Text="AOT is not supported in debug configuration (Configuration=Release required)." />
<Error Condition="'@(_WasmAssembliesInternal)' == ''" Text="Item _WasmAssembliesInternal is empty" />
<Error Condition="'$(_IsToolchainMissing)' == 'true'"
Text="$(_ToolchainMissingErrorMessage) SDK is required for AOT'ing assemblies." />
Expand Down
Loading