Use `AppContext.BaseDirectory` #993

kant2002 · 2021-09-15T10:30:46Z

instead of Assembly.GetExecutingAssembly().Location.
I change this pattern in tests too, in case somebody would like to execute them after trimming or AOT context
Related #992

manodasanW · 2021-09-15T16:52:54Z

/azp run

azure-pipelines · 2021-09-15T16:52:59Z

No pipelines are associated with this pull request.

manodasanW · 2021-09-15T18:42:53Z

@kant2002 Thanks for addressing this. I ran the pipeline and it looks like the following test failed. Can you check if it passes for you or not? Might be related to the trailing slash when appending filename.

[xUnit.net 00:00:10.33] UnitTest.TestCSharp.TestStorageFile [FAIL]
Failed UnitTest.TestCSharp.TestStorageFile [32 ms]
Error Message:
System.AggregateException : One or more errors occurred. (Value does not fall within the expected range.)
---- System.ArgumentException : Value does not fall within the expected range.
Stack Trace:
at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions) in System.Private.CoreLib.dll:token 0x6002dd6+0x11
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken) in System.Private.CoreLib.dll:token 0x6002df2+0x3f
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout) in System.Private.CoreLib.dll:token 0x6002df1+0x0
at UnitTest.TestCSharp.TestStorageFile() in D:\a\1\s\src\Tests\UnitTest\TestComponentCSharp_Tests.cs:line 540
----- Inner Stack Trace -----
at UnitTest.TestCSharp.TestStorageFileAsync() in D:\a\1\s\src\Tests\UnitTest\TestComponentCSharp_Tests.cs:line 532

tlakollo · 2021-09-20T20:28:18Z

src/Tests/UnitTest/TestComponentCSharp_Tests.cs

@@ -528,7 +528,7 @@ public void TestBuffer()

        async Task TestStorageFileAsync()
        {
-            var folderPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location);
+            var folderPath = AppContext.BaseDirectory;
            StorageFile file = await StorageFile.GetFileFromPathAsync(folderPath + "\\UnitTest.dll");


This is the problematic line the AppContext.BaseDirectory already contains a forward slash at the end of the path, for example cswinrt\src\Tests\unittest\bin\x64\Release\net6.0\
Changing the code to

StorageFile file = await StorageFile.GetFileFromPathAsync($"{folderPath}UnitTest.dll");

worked for me.
Having said that I think we might need something to test even this same test storage test in single file mode. I think files like the Benchmarks.manifest will not be bundled and getting the base directory and then looking for the file will work. But I'm not completely sure about UnitTest.dll if it gets bundled along with the other dlls you might not find it.

Does UnitTest.dll used just because it happens to exists in the build of the test project? Maybe test can have some non-executable file which will be read. Or this code tests that executable file can be read?

tlakollo · 2021-09-20T20:34:45Z

src/cswinrt/strings/WinRT.cs

@@ -107,7 +107,7 @@ internal class DllModule
        readonly DllGetActivationFactory _GetActivationFactory;
        readonly DllCanUnloadNow _CanUnloadNow; // TODO: Eventually periodically call

-        static readonly string _currentModuleDirectory = System.IO.Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location);
+        static readonly string _currentModuleDirectory = AppContext.BaseDirectory;


I'm not fully sure if this is the right fix, in single-file mode the AppContext.BaseDirectory will point to the location of the single-file executable. Depending on how the _currentModuleDirectory is used later, the code could break which is why I opened the issue. If someone uses the _currentModuleDirectory to find stuff that got bundled in the exe that is going to break
cc @agocke

The only usage for that variable is here

CsWinRT/src/cswinrt/strings/WinRT.cs

Lines 133 to 135 in 755b8a7

// Explicitly look for module in the same directory as this one, and

// use altered search path to ensure any dependencies in the same directory are found.

var moduleHandle = Platform.LoadLibraryExW(System.IO.Path.Combine(_currentModuleDirectory, fileName), IntPtr.Zero, /* LOAD_WITH_ALTERED_SEARCH_PATH */ 8);

Which is just used to get DllGetActivationFactory from DLL. That's for fallback, if RoGetActivationFactory cannot find factory for the class. And you are prbably right, I'm too fast to apply fix here.

Do I properly understand that DllGetActivationFactory is created in DLL by WinRT-component vendor. In case if CsWinRT was used it is comes from here

CsWinRT/src/Authoring/WinRT.Host/WinRT.Host.cpp

Lines 300 to 303 in ad6adcb

// Assumes the managed assembly to load and its runtime configuration file are next to the host

wchar_t buffer[MAX_PATH];

auto size = ::GetModuleFileName((HINSTANCE)&__ImageBase, buffer, _countof(buffer));

std::filesystem::path host_module(buffer);

Short glance at the code indicates that it does not take into account single-file deployment at all. Do I see things correctly?

At least the assumption that managed assemblies and the runtime configuration will be next to the host is not true per the documentation https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file they are both going to be bundled into the single file.

My understanding is that the code is loading a native dll from that path. Native dlls don't get bundled by default, so using the BaseDirectory will work in that case. Unfortunately if somebody specifies IncludeNativeLibrariesForSelfExtract the native dll will be bundled and then extracted into a "Temp" directory. But currently we don't provide a good way to find that directory - see dotnet/runtime#43010.

I think one of the native library APIs should be able to try to load the library by a simple name (the same way DllImport does), which will find the library always, since the host correctly sets up NATIVE_DLL_SEARCH_DIRECTORIES. @elinor-fung Do you know which of the NativeLibrary APIs (if any) will trigger the normal runtime's "load library" behavior.

NativeLibrary.Load(string, Assembly, DllImportSearchPath?) or NativeLibrary.TryLoad(string, Assembly, DllImportSearchPath?)

I probably misled people a bit here by not showing subsequent code, with assumption that it would be inspected. See below.

CsWinRT/src/cswinrt/strings/WinRT.cs

Lines 133 to 141 in 755b8a7

// Explicitly look for module in the same directory as this one, and

// use altered search path to ensure any dependencies in the same directory are found.

var moduleHandle = Platform.LoadLibraryExW(System.IO.Path.Combine(_currentModuleDirectory, fileName), IntPtr.Zero, /* LOAD_WITH_ALTERED_SEARCH_PATH */ 8);

#if !NETSTANDARD2_0 && !NETCOREAPP2_0

if (moduleHandle == IntPtr.Zero)

{

NativeLibrary.TryLoad(fileName, Assembly.GetExecutingAssembly(), null, out moduleHandle);

}

#endif

shows that Platform.LoadLibraryExW used primarily for NETSTANDARD2.0 or NETCOREAPP2.0, when on more modern platforms subsequently used NativeLibrary.TryLoad which is essentially same as what @elinor-fung proposed. If dllImportSearchPath == null trigger use NATIVE_DLL_SEARCH_DIRECTORIES then probably that's enough.

Only case which I think is left, is when somehow somebody would like to have "out-of-process" hosting with single-file package. That's WinRT.Host.cpp file is for which I mention in previous comments. Is this supported scenario?

Single-file is only supported for applications (exe), I would not expect out-of-proc hosted components to be built as applications (similar to COM host components, those are also classlib projects not apps). So that should already disqualify almost all such cases.

On top of that, hosting APIs (hostfxr) don't support single-file in any form currently, so even if the SDK would somehow build it, it won't run.

kant2002 · 2021-09-26T05:54:23Z

@tlakollo do you have any other concerns? It seems that everything was sorted out in the end. Isn't it?

tlakollo · 2021-09-27T18:04:41Z

From what I read the scenario that uses Assembly.Location is not supported in single-file mode, so I'm fine with either the current change just run the CI and merge or leave the old code and just suppress the warning with the justification that mentions the code won't reach that path in single-file mode. Let me see if I have permission to run the pipeline which I doubt

tlakollo · 2021-09-27T18:04:49Z

/azp run

azure-pipelines · 2021-09-27T18:04:54Z

Commenter does not have sufficient privileges for PR 993 in repo microsoft/CsWinRT

tlakollo · 2021-09-27T18:06:27Z

@manodasanW can you trigger the pipeline run to verify the change doesn't break anything?

manodasanW · 2021-09-28T02:07:58Z

/azp run

manodasanW · 2021-09-28T03:32:52Z

Looks like all tests passed, but none of the tests I believe today exercise single file mode. We should try out a CsWinRT projection consumption scenario with single file mode and ensure that it works with these changes. @angelazhangmsft can you help out here trying this build with one of our console app samples?

kant2002 · 2021-09-28T03:34:32Z

Isn’t proper way would be start working on running single-file mode tests? Or there already issue which tracks that?

…

On Tue, Sep 28, 2021 at 09:33 Manodasan Wignarajah ***@***.***> wrote: Looks like all tests passed, but none of the tests I believe today exercise single file mode. We should try out a CsWinRT projection consumption scenario with single file mode and ensure that it works with these changes. @angelazhangmsft <https://github.com/angelazhangmsft> can you help out here trying this build with one of our console app samples? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#993 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAPKNYQY2EBUTPFUNY2KFTUEEZPBANCNFSM5ECDZXSQ> .

manodasanW · 2021-09-28T03:41:40Z

@kant2002 I agree we should add some single file test scenarios to validate this. From a quick look, it wasn't clear to me if we would be able to get our MSTest tests to run also in single file mode. But we can probably have a console app consuming a CsWinRT test projection that publishes to single file and then run it in our test pass as a form of validating it. Thoughts or any other suggestions?

angelazhangmsft · 2021-09-29T00:45:54Z

Looks like all tests passed, but none of the tests I believe today exercise single file mode. We should try out a CsWinRT projection consumption scenario with single file mode and ensure that it works with these changes. @angelazhangmsft Angela Zhang (SHE/HER) FTE can you help out here trying this build with one of our console app samples?

Looks like the changes here enable CsWinRT projection consumption scenario with single file mode. We should address #999 before merging the PR

instead of Assembly.GetExecutingAssembly().Location. Related microsoft#992

manodasanW · 2022-07-25T20:45:53Z

/azp run

azure-pipelines · 2022-07-25T20:46:12Z

Azure Pipelines successfully started running 1 pipeline(s).

kant2002 · 2022-07-26T05:32:40Z

Thanks you!

instead of Assembly.GetExecutingAssembly().Location. Related #992 Co-authored-by: Manodasan Wignarajah <[email protected]>

tlakollo reviewed Sep 20, 2021

View reviewed changes

kant2002 mentioned this pull request Sep 23, 2021

WinRT file uses single file dangerous pattern #992

Closed

microsoft deleted a comment from azure-pipelines bot Sep 28, 2021

kant2002 force-pushed the kant/issue-922 branch from 64e72a1 to 10ed0a1 Compare April 15, 2022 07:40

Use AppContext.BaseDirectory

b679996

instead of Assembly.GetExecutingAssembly().Location. Related microsoft#992

kant2002 force-pushed the kant/issue-922 branch from e5aeef0 to b679996 Compare April 15, 2022 07:43

kant2002 mentioned this pull request Jun 2, 2022

DllModule does not work with PublishSingleFile #1168

Closed

kant2002 mentioned this pull request Jun 17, 2022

Single file mode tests #999

Closed

Merge branch 'master' into kant/issue-922

8d4066a

manodasanW mentioned this pull request Jul 22, 2022

Make the functional tests single file to test single file support #1226

Merged

manodasanW approved these changes Jul 22, 2022

View reviewed changes

j0shuams approved these changes Jul 25, 2022

View reviewed changes

manodasanW merged commit 06b377d into microsoft:master Jul 26, 2022

kant2002 deleted the kant/issue-922 branch July 26, 2022 05:32

manodasanW added a commit that referenced this pull request Jul 29, 2022

Use AppContext.BaseDirectory (#993)

182d86a

instead of Assembly.GetExecutingAssembly().Location. Related #992 Co-authored-by: Manodasan Wignarajah <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `AppContext.BaseDirectory` #993

Use `AppContext.BaseDirectory` #993

kant2002 commented Sep 15, 2021 •

edited

Loading

manodasanW commented Sep 15, 2021

azure-pipelines bot commented Sep 15, 2021

manodasanW commented Sep 15, 2021

tlakollo Sep 20, 2021

kant2002 Sep 21, 2021

tlakollo Sep 20, 2021

kant2002 Sep 21, 2021

tlakollo Sep 23, 2021

vitek-karas Sep 23, 2021

elinor-fung Sep 23, 2021 •

edited

Loading

kant2002 Sep 23, 2021

vitek-karas Sep 24, 2021

kant2002 commented Sep 26, 2021

tlakollo commented Sep 27, 2021

tlakollo commented Sep 27, 2021

azure-pipelines bot commented Sep 27, 2021

tlakollo commented Sep 27, 2021

manodasanW commented Sep 28, 2021

manodasanW commented Sep 28, 2021

kant2002 commented Sep 28, 2021 via email

manodasanW commented Sep 28, 2021

angelazhangmsft commented Sep 29, 2021

manodasanW commented Jul 25, 2022

azure-pipelines bot commented Jul 25, 2022

kant2002 commented Jul 26, 2022

	// Explicitly look for module in the same directory as this one, and
	// use altered search path to ensure any dependencies in the same directory are found.
	var moduleHandle = Platform.LoadLibraryExW(System.IO.Path.Combine(_currentModuleDirectory, fileName), IntPtr.Zero, /* LOAD_WITH_ALTERED_SEARCH_PATH */ 8);

	// Assumes the managed assembly to load and its runtime configuration file are next to the host
	wchar_t buffer[MAX_PATH];
	auto size = ::GetModuleFileName((HINSTANCE)&__ImageBase, buffer, _countof(buffer));
	std::filesystem::path host_module(buffer);

Use AppContext.BaseDirectory #993

Use AppContext.BaseDirectory #993

Conversation

kant2002 commented Sep 15, 2021 • edited Loading

manodasanW commented Sep 15, 2021

azure-pipelines bot commented Sep 15, 2021

manodasanW commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kant2002 commented Sep 26, 2021

tlakollo commented Sep 27, 2021

tlakollo commented Sep 27, 2021

azure-pipelines bot commented Sep 27, 2021

tlakollo commented Sep 27, 2021

manodasanW commented Sep 28, 2021

manodasanW commented Sep 28, 2021

kant2002 commented Sep 28, 2021 via email

manodasanW commented Sep 28, 2021

angelazhangmsft commented Sep 29, 2021

manodasanW commented Jul 25, 2022

azure-pipelines bot commented Jul 25, 2022

kant2002 commented Jul 26, 2022

Use `AppContext.BaseDirectory` #993

Use `AppContext.BaseDirectory` #993

kant2002 commented Sep 15, 2021 •

edited

Loading

elinor-fung Sep 23, 2021 •

edited

Loading