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

Make PowerShell language worker process match the bitness of the host #5011

Merged
merged 9 commits into from
Oct 15, 2019

Conversation

AnatoliB
Copy link
Contributor

@AnatoliB AnatoliB commented Oct 2, 2019

Addressing Azure/azure-functions-powershell-worker#232

Currently, Functions Host starts the PowerShell language worker by using the dotnet command without specifying the full path. As a result, it always starts the 32-bit version of dotnet on Consumption, simply because D:\Program Files (x86)\dotnet happens to be on the PATH environment variable configured on the sandbox. The platform setting on the app is completely ignored.

This fix makes the Host invoke dotnet from the ProgramFiles folder (e.g. D:\Program Files\dotnet or D:\Program Files (x86)\dotnet, depending on the current process), so that the bitness of the PowerShell worker process matches the bitness of the Host.

The fix is intentionally applied to PowerShell only. Performing the same for other dotnet-based languages may be desirable, but needs to be considered carefully.

Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. @maiqbal11 recently added support to specify a format for defaultWorkerPath Please see changes in the PR##4985

We should follow similar approach and add a format string %bitness% for defaultExecutablePath. Let's sync up offline if you need more details.

@AnatoliB
Copy link
Contributor Author

@pragnagopa Thank you. This approach (if I understood it correctly) may not be applicable in situations when we are not in control of the directory structure, and the dotnet.exe case seems to be one of them. Let's discuss offline.

@ghost ghost removed the Needs: Author Feedback label Oct 11, 2019
@AnatoliB
Copy link
Contributor Author

@pragnagopa I've also realized that this change would probably not work on Linux, so I'm not going to merge this as is in any case, but let's discuss the proper way.

Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

Added minor comments

const string DotNetExecutableName = "dotnet";
const string DotNetFolderName = "dotnet";

if (DefaultExecutablePath == DotNetExecutableName && RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Copy link
Member

Choose a reason for hiding this comment

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

Use case insensitive comparison


private void ResolveDotNetDefaultExecutablePath()
{
const string DotNetExecutableName = "dotnet";
Copy link
Member

Choose a reason for hiding this comment

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

move these to LanguageWorkerConstants file

@@ -65,6 +65,7 @@ public IList<WorkerConfig> GetConfigs()
{
arguments.ExecutablePath = GetExecutablePathForJava(description.DefaultExecutablePath);
}

Copy link
Member

Choose a reason for hiding this comment

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

undo file changes


[Theory]
[InlineData("DotNet")]
[InlineData("dotnet.exe")]
Copy link
Member

Choose a reason for hiding this comment

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

we should allow dotnet.exe as running dotnet.exe is same as running dotnet on windows

@AnatoliB
Copy link
Contributor Author

As per our discussion:

  • making the fix apply to any worker using "dotnet" as defaultExecutablePath, removed the PowerShell-specific check;
  • moving the path resolution logic from WorkerConfigFactory to RpcWorkerDescription.

See also: #5073

Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

👍

@AnatoliB AnatoliB merged commit 8711977 into Azure:dev Oct 15, 2019
@AnatoliB AnatoliB deleted the powershell-match-host-bitness branch October 15, 2019 20:10
maiqbal11 pushed a commit to maiqbal11/azure-functions-host that referenced this pull request Oct 16, 2019
…Azure#5011)

Make out-of-proc dotnet-based language worker processes match the bitness of the host process
fabiocav pushed a commit that referenced this pull request Apr 22, 2020
…#5011)

Make out-of-proc dotnet-based language worker processes match the bitness of the host process
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants