allow python venv path as env var w/ PythonProcessOptions#12817
allow python venv path as env var w/ PythonProcessOptions#12817l0lawrence merged 16 commits intoAzure:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying a Python virtual environment path when running package validation checks. This allows users to run Python-specific checks (like snippet updates, linting, and formatting) using tools from a specific virtual environment instead of relying on system-wide installations.
Key changes:
- Added a new
--python-venv-pathcommand-line option to the package check tool - Implemented logic to resolve Python and tool executables from the specified virtual environment
- Updated method signatures to pass the Python venv path through the call chain
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
PackageCheckTool.cs |
Added --python-venv-path option and plumbed it through command handling to the Python language checks |
PythonLanguageSpecificChecks.cs |
Implemented venv path resolution for Python executables and tools, with fallback to system Python |
CheckAllToolTests.cs |
Updated test calls to match the new method signature (removed explicit CancellationToken.None parameter) |
Comments suppressed due to low confidence (1)
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageSpecificChecks.cs:1
- The new Python venv path functionality lacks test coverage. Consider adding tests that verify: (1) the venv path is correctly set when provided, (2) Python and tool executables are resolved from the venv directory, (3) fallback to system Python occurs when executables are not found in venv, and (4) the
DirectoryNotFoundExceptionis thrown when an invalid venv path is provided. Other language-specific checks in the codebase have dedicated test files (e.g.,DotNetLanguageSpecificChecksTests.cs,JavaLanguageSpecificChecksTests.cs).
using Azure.Sdk.Tools.Cli.Helpers;
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageSpecificChecks.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Tools/Package/PackageCheckTool.cs
Outdated
Show resolved
Hide resolved
01a4bac to
94cb8bb
Compare
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
richardpark-msft
left a comment
There was a problem hiding this comment.
Seems basically okay, but it's lacking some maintainability - there's hardly any comments explaining things, there's a global variable in there, and there's no tests.
I think it's worth putting in the time here when it's still fresh on your mind.
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonOptions.cs
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.Checks.cs
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.Checks.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.Checks.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.Checks.cs
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.cs
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.Checks.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PythonLanguageService.Checks.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/Process/Options/PythonProcessOptions.cs
Outdated
Show resolved
Hide resolved
richardpark-msft
left a comment
There was a problem hiding this comment.
Talked with @l0lawrence about the environment variable - it's a simple workaround to allow advanced users to use venvs, if they want, but it's not the mainline scenario we expect developers to use.
Approving.
User would need to set environment variable AZSDKTOOLS_PYTHON_VENV_PATH to be their desired venv path otherwise we default back to the python path if it is not set -- as we develop this can turn into creating a venv + installing the required dependencies for the verifySetup step