fix!: Windows locate_executable finds wrong binary to run

mwiebe commented 4 months ago

What was the problem/requirement? (What/Why)

This fixes issue #140

What was the solution? (How)

By calling a Python subprocess to run shutil.where instead of a cmd subprocess to run where.

What is the impact of this change?

Jobs depending on binaries provided by environments will reliably find the correct one from the PATH.

How was this change tested?

Installed this change on the Windows worker host where it was failing with the following command:

& C:\Programs\Conda\python -m pip install --no-deps --force-reinstall git+https://github.com/mwiebe/openjd-sessions-for-python.git@win-where

Then restarted the DeadlineWorker service, and tested the jobs that were failing previous to the fix. Observed reliable successful runs.

While improving the error message output, I ran the following commands to remove permissions of the job user to access the Python installation as follows:

PS C:\Programs\Conda> icacls . /T /Q /inheritance:d
Successfully processed 14568 files; Failed processing 0 files
PS C:\Programs\Conda> icacls . /T /Q /remove "BUILTIN\Users"
Successfully processed 14568 files; Failed processing 0 files

Then I confirmed that the resulting log output included the message with more details about the username, the executable it tried to run, and the output the process produced. I then restored the inherited ACLs, and confirmed a job that ran successfully. Finally, I tested a job with a non-existent binary name, and confirmed the simple error message case.

Was this change documented?

The new requirement related to this being a breaking change was added to the README.

Is this a breaking change?

It is a breaking change, because it introduces a new requirement that when impersonating a user for subprocesses, the Python installation hosting the library can be run by the impersonated user as well.

I believe it is likely that the new requirement is already met in existing usage, as was the case for my setup.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

mwiebe commented 4 months ago

I've updated the code to share that path_var logic between the two cases.