Skip to content

[Wasm]Adjust AppContext.BaseDirectory and enable Microsoft.Extensions tests #38721

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
steveisok merged 6 commits into from
Jul 7, 2020
Merged

[Wasm]Adjust AppContext.BaseDirectory and enable Microsoft.Extensions tests #38721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f67297
Return / in AppContext.BaseDirectory for wasm
Jun 29, 2020
d7960c6
Adjust AppContext.BaseDirectory to return "/" for wasm because GetEnt…
Jul 2, 2020
3f7e5a7
Format tweak
Jul 2, 2020
db4159e
Skip test that depends on FSW
Jul 6, 2020
f6ad87e
Feedback
Jul 6, 2020
3dcbda4
Whitespace :-)
Jul 6, 2020
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
3 changes: 2 additions & 1 deletion src/libraries/Microsoft.Extensions.Configuration.Xml/tests/XmlConfigurationTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,8 @@ public void XmlConfiguration_Does_Not_Throw_On_Optional_Configuration()
var config = new ConfigurationBuilder().AddXmlFile("NotExistingConfig.xml", optional: true).Build();
}

[ConditionalFact]
[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/37669", TestPlatforms.Browser)]
[FrameworkSkipCondition(RuntimeFrameworks.Mono)]
Copy link
Member

@akoeplinger akoeplinger Jul 7, 2020

Choose a reason for hiding this comment

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

I think this can be removed as well, it doesn't do anything useful anymore since this is about the old .NET Framework-based Mono. We can do that in a separate PR, filed #38878.

public void LoadKeyValuePairsFromValidEncryptedXml()
{
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Microsoft.Extensions.Configuration/tests/FunctionalTests/ConfigurationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,7 @@ public void GetDefaultBasePathForSources()

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/34580", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
[SkipOnMono("System.IO.FileSystem.Watcher is not supported on wasm", TestPlatforms.Browser)]
public void CanEnumerateProviders()
{
var config = CreateBuilder()
Expand Down
8 changes: 8 additions & 0 deletions src/libraries/Microsoft.Extensions.DependencyModel/tests/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using Xunit;

[assembly: ActiveIssue("https://github.com/dotnet/runtime/issues/37669", TestPlatforms.Browser)]
8 changes: 8 additions & 0 deletions src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using Xunit;

[assembly: ActiveIssue("https://github.com/dotnet/runtime/issues/37669", TestPlatforms.Browser)]
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Activator.RuntimeType.cs" Condition="'$(TargetsCoreRT)' != 'true'" />
<Compile Include="$(MSBuildThisFileDirectory)System\AggregateException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppContext.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppContext.Browser.cs" Condition="'$(TargetsBrowser)' == 'true'" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppContext.AnyOS.cs" Condition="'$(TargetsBrowser)' != 'true'" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppDomain.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppDomainSetup.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\AppDomainUnloadedException.cs" />
Expand Down
26 changes: 26 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/AppContext.AnyOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.IO;
using System.Reflection;

namespace System
{
public static partial class AppContext
{
private static string GetBaseDirectoryCore()
{
// Fallback path for hosts that do not set APP_CONTEXT_BASE_DIRECTORY explicitly
string? directory = Path.GetDirectoryName(Assembly.GetEntryAssembly()?.Location);

if (directory == null)
return string.Empty;

if (!Path.EndsInDirectorySeparator(directory))
directory += PathInternal.DirectorySeparatorCharAsString;

return directory;
}
}
}
16 changes: 16 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/AppContext.Browser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace System
{
public static partial class AppContext
{
private static string GetBaseDirectoryCore()
{
// GetEntryAssembly().Location returns an empty string for wasm
// Until that can be easily changed, work around the problem here.
Copy link
Member

@jkotas jkotas Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How is this working around GetEntryAssembly().Location returning empty string?

Would it make more sense for BaseDirectory to be empty string as well when there is no file system?

Copy link
Member Author

@steveisok steveisok Jul 3, 2020

Choose a reason for hiding this comment

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

Perhaps the wording is incorrect. AppContext.BaseDirectory is returning "", which is causing a bunch of tests to fail. I feel returning "/" isn't any less wrong than "". The benefit of "/" being that it works within other parts of libraries.

Copy link
Member

@jkotas jkotas Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that returning "/" is more wrong than "".

If we were optimizing for making maximum tests to pass without any changes, we would also change Assembly.Location to return /<assemblyname>.dll that would be even more wrong.

What are the tests that fail before this change and pass with this change?

Copy link
Member Author

@steveisok steveisok Jul 3, 2020

Choose a reason for hiding this comment

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

I’ll follow up with a list.

Since the emscripten vfs mounts at /, I think my change does make sense.
https://emscripten.org/docs/porting/files/file_systems_overview.html

Copy link
Member

@jkotas jkotas Jul 4, 2020

Choose a reason for hiding this comment

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

Do the application binaries show up as files at / ?

The docs for AppContext.BaseDirectory say: Gets the pathname of the base directory that the assembly resolver uses to probe for assemblies.

Copy link
Member Author

@steveisok steveisok Jul 6, 2020

Choose a reason for hiding this comment

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

Here are the tests that fail expecting some sort of value out of BaseDirectory.

Regarding application binaries, my understanding is that we load everything essentially from an in-memory byte array. That strikes me as an implementation detail though. If the user, for example, downloads Foo.dll and stores it in the VFS, probing at /Foo.dll would be expected.

I still think setting "/" is the way to go. That said, there definitely does seem to be a bunch of little hosting twists that we should try to understand / surface better.

Copy link
Member

@jkotas jkotas Jul 6, 2020

Choose a reason for hiding this comment

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

Do the types exercised by there tests actually work on Wasm Browser? E.g. can I add configuration file to my .csproj file and then get the file picked up by the default IFileProvider at runtime?

Copy link
Member Author

@steveisok steveisok Jul 6, 2020

Choose a reason for hiding this comment

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

Good question. If you look at the test results, we're likely at least somewhat functional.

https://gist.github.com/steveisok/370622cdb0c41ff2bc3b439c730484e4

Copy link
Member

@jkotas jkotas Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should have issues opened on making sure that the end-to-end works for these libraries. More passing tests does not necessarily mean that these libraries are actually usable by customers.

The linker analysis for single file unfriendly APIs (#38405) should help us get a better handle on this eventually.

If you believe that returning / here is going to help to make the affected scenarios work end-to-end, I can live with it.

return "/";
Copy link
Contributor

@marek-safar marek-safar Jul 2, 2020

Choose a reason for hiding this comment

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

Should we add explicit test for this?

Copy link
Member Author

@steveisok steveisok Jul 2, 2020

Choose a reason for hiding this comment

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

Good question. I think it's acceptable to just rely on the tests that use this property to pass.

}
}
}
9 changes: 0 additions & 9 deletions src/libraries/System.Private.CoreLib/src/System/AppContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,6 @@ internal static unsafe void Setup(char** pNames, char** pValues, int count)
s_dataStore.Add(new string(pNames[i]), new string(pValues[i]));
}
}

private static string GetBaseDirectoryCore()
{
// Fallback path for hosts that do not set APP_CONTEXT_BASE_DIRECTORY explicitly
string? directory = Path.GetDirectoryName(Assembly.GetEntryAssembly()?.Location);
if (directory != null && !Path.EndsInDirectorySeparator(directory))
directory += PathInternal.DirectorySeparatorCharAsString;
return directory ?? string.Empty;
}
#endif
}
}
8 changes: 0 additions & 8 deletions src/libraries/tests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,6 @@
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Net.WebSockets.WebSocketProtocol\tests\System.Net.WebSockets.WebSocketProtocol.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Text.Json\tests\System.Text.Json.Tests.csproj" />
<!-- Builds currently do not pass -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Configuration.FileExtensions\tests\Microsoft.Extensions.Configuration.FileExtensions.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Configuration.Ini\tests\Microsoft.Extensions.Configuration.Ini.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Configuration.Json\tests\Microsoft.Extensions.Configuration.Json.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Configuration.UserSecrets\tests\Microsoft.Extensions.Configuration.UserSecrets.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Configuration.Xml\tests\Microsoft.Extensions.Configuration.Xml.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Configuration\tests\FunctionalTests\Microsoft.Extensions.Configuration.Functional.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.DependencyModel\tests\Microsoft.Extensions.DependencyModel.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Hosting\tests\UnitTests\Microsoft.Extensions.Hosting.Unit.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.VisualBasic.Core\tests\Microsoft.VisualBasic.Core.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.CodeDom\tests\System.CodeDom.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Collections.NonGeneric\tests\System.Collections.NonGeneric.Tests.csproj" />
Expand Down