[Wasm]Adjust AppContext.BaseDirectory and enable Microsoft.Extensions tests #38721
Conversation
…ryAssembly().Location returns an empty string. This change also enables most of the Microsoft.Extensions.* tests with the exception of DependencyModel and Hosting.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| { | ||
| // 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)) |
There was a problem hiding this comment.
Do the null check only one and return Empty here
| { | ||
| // GetEntryAssembly().Location returns an empty string for wasm | ||
| // Until that can be easily changed, work around the problem here. | ||
| return "/"; |
There was a problem hiding this comment.
Should we add explicit test for this?
There was a problem hiding this comment.
Good question. I think it's acceptable to just rely on the tests that use this property to pass.
| private static string GetBaseDirectoryCore() | ||
| { | ||
| // GetEntryAssembly().Location returns an empty string for wasm | ||
| // Until that can be easily changed, work around the problem here. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good question. If you look at the test results, we're likely at least somewhat functional.
https://gist.github.com/steveisok/370622cdb0c41ff2bc3b439c730484e4
There was a problem hiding this comment.
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.
src/libraries/Microsoft.Extensions.DependencyModel/tests/AssemblyInfo.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated
Show resolved
Hide resolved
| [ConditionalFact] | ||
| [Fact] | ||
| [ActiveIssue("https://github.com/dotnet/runtime/issues/37669", TestPlatforms.Browser)] | ||
| [FrameworkSkipCondition(RuntimeFrameworks.Mono)] |
There was a problem hiding this comment.
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.
Adjust AppContext.BaseDirectory to return "/" for wasm because GetEntryAssembly().Location returns an empty string.
This change also enables most of the Microsoft.Extensions.* tests with the exception of DependencyModel and Hosting.