Skip to content
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

chore: respect CodeBase when locating driver entrypoint #2266

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Aug 22, 2022

Fixes #2241
Fixes #2004
Fixes #2240

@@ -37,7 +37,19 @@ internal static string GetExecutablePath()
DirectoryInfo assemblyDirectory = new(AppContext.BaseDirectory);
if (!assemblyDirectory.Exists || !File.Exists(Path.Combine(assemblyDirectory.FullName, "Microsoft.Playwright.dll")))
{
#if NETFRAMEWORK
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if that works for a project targeting netstandard2.0 only. Also I guess that this should be negated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. This will always be false.

Copy link
Member Author

@mxschmitt mxschmitt Aug 22, 2022

Choose a reason for hiding this comment

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

Yeah, it's wrong, we should somehow check during the runtime if the application is NETFRAMEWORK since its always netstandard as of today when we do it via preprocessor directives, alternatively add .NET Framework 2.0 to the tfm.

Copy link
Member

Choose a reason for hiding this comment

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

Is the value different between .NET and .NET Framework? Maybe you can just always use CodeBase?

You could check on runtime, but it is a bit annoying because the versions don't correspond exactly to the .NET versions. https://stackoverflow.com/questions/1826688/get-current-net-clr-version-at-runtime

@mxschmitt mxschmitt force-pushed the respect-codebase branch 2 times, most recently from a1823ac to f1702e5 Compare August 23, 2022 09:35
@nohwnd
Copy link
Member

nohwnd commented Aug 23, 2022

The reason to use CodeBase is that that asp.net shadow copies the dll into temp, and so we need to locate the actual dll to find the .playwright folder rather than then copy in temp. Similar issue is a common stumbling point in Xunit, with the same suggested workaround: https://stackoverflow.com/a/283917/3065397

@mxschmitt mxschmitt merged commit 2c8b74c into microsoft:main Aug 23, 2022
Comment on lines +40 to +51
string assemblyLocation = null;
#pragma warning disable SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility.
var assemblyLocationFileUrl = typeof(Playwright).Assembly.CodeBase;
#pragma warning restore SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility.
if (!string.IsNullOrEmpty(assemblyLocationFileUrl))
{
assemblyLocation = new Uri(assemblyLocationFileUrl).LocalPath;
}
else
{
assemblyLocation = typeof(Playwright).Assembly.Location;
}
Copy link
Contributor

@campersau campersau Aug 23, 2022

Choose a reason for hiding this comment

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

Suggested change
string assemblyLocation = null;
#pragma warning disable SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility.
var assemblyLocationFileUrl = typeof(Playwright).Assembly.CodeBase;
#pragma warning restore SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility.
if (!string.IsNullOrEmpty(assemblyLocationFileUrl))
{
assemblyLocation = new Uri(assemblyLocationFileUrl).LocalPath;
}
else
{
assemblyLocation = typeof(Playwright).Assembly.Location;
}
string assemblyLocation;
var assembly = typeof(Playwright).Assembly;
#pragma warning disable SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility.
if (Uri.TryCreate(assembly.CodeBase, UriKind.Absolute, out var codeBase) && codeBase.IsFile)
#pragma warning restore SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility.
{
assemblyLocation = codeBase.LocalPath;
}
else
{
assemblyLocation = assembly.Location;
}

Slightly more paranoid version as new Uri could throw.
The same logic is also used by Microsoft.Extensions.DependencyModel:
https://github.com/dotnet/runtime/blob/e55c908229e36f99a52745d4ee85316a0e8bb6a2/src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextLoader.cs#L161

But they are using CodeBase as a fallback instead and don't prefer it over Assembly.Location like here.
https://github.com/dotnet/runtime/blob/e55c908229e36f99a52745d4ee85316a0e8bb6a2/src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextLoader.cs#L128-L150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants