-
Notifications
You must be signed in to change notification settings - Fork 28
feat(python): auto-activate venv before hook execution #347
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
Changes from all commits
9d6e1d9
665e23c
9008d02
51030ee
6e6ba42
fdd8e15
cf35c5c
bff1d8c
b0f0050
e641b20
4324103
3a76ba8
180d499
253ee65
adbda1b
91a07bf
b75edb9
ce15297
f8755c9
8554adc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,45 @@ func getPythonExecutable() string { | |
| return "python3" | ||
| } | ||
|
|
||
| // getVenvBinDir returns the platform-specific bin directory inside a virtual environment | ||
| func getVenvBinDir(venvPath string) string { | ||
| if runtime.GOOS == "windows" { | ||
| return filepath.Join(venvPath, "Scripts") | ||
| } | ||
| return filepath.Join(venvPath, "bin") | ||
| } | ||
|
|
||
| // ActivateVenvIfPresent sets the process environment variables that a Python | ||
| // virtual environment's activate script would set, so that child processes | ||
| // (hook scripts) inherit the activated venv. This is a no-op when no .venv | ||
| // directory exists in projectDir. | ||
| // | ||
| // The three env vars (VIRTUAL_ENV, PATH, PYTHONHOME) are the stable contract | ||
| // defined by PEP 405 (Python 3.3, 2012). Other tools (Poetry, tox, pipx) use | ||
| // the same approach. Sourcing the shell-specific activate script (activate, | ||
| // activate.fish, Activate.ps1) would be higher maintenance because it varies | ||
| // by shell. | ||
| func ActivateVenvIfPresent(fs afero.Fs, osClient types.Os, projectDir string) (bool, error) { | ||
| venvPath := getVenvPath(projectDir) | ||
| if !venvExists(fs, venvPath) { | ||
| return false, nil | ||
| } | ||
|
Comment on lines
+99
to
+101
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🐍 question: Do we want to avoid reactivating a virtual environment if one is present? This might be an issue if different environments are activated for different reasons, but I'm unsure if that's a common practice. I'm alright merging this without a clear answer to extend these approaches based on feedback! The happy path works so well and |
||
|
|
||
| binDir := getVenvBinDir(venvPath) | ||
|
|
||
| if err := osClient.Setenv("VIRTUAL_ENV", venvPath); err != nil { | ||
| return false, err | ||
| } | ||
| if err := osClient.Setenv("PATH", binDir+string(filepath.ListSeparator)+osClient.Getenv("PATH")); err != nil { | ||
| return false, err | ||
| } | ||
| if err := osClient.Unsetenv("PYTHONHOME"); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| // getPipExecutable returns the path to the pip executable in the virtual environment | ||
| func getPipExecutable(venvPath string) string { | ||
| if runtime.GOOS == "windows" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,14 @@ func New(runtimeName string) (Runtime, error) { | |
| return rt, nil | ||
| } | ||
|
|
||
| // ActivatePythonVenvIfPresent activates a Python virtual environment if one | ||
| // exists in the given project directory. This sets VIRTUAL_ENV, prepends the | ||
| // venv bin directory to PATH, and unsets PYTHONHOME on the current process so | ||
| // that child processes (hook scripts) inherit the activated venv. | ||
| func ActivatePythonVenvIfPresent(fs afero.Fs, os types.Os, projectDir string) (bool, error) { | ||
| return python.ActivateVenvIfPresent(fs, os, projectDir) | ||
| } | ||
|
|
||
|
Comment on lines
69
to
76
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: While it feels wrong to have Python in the generic Upside of this approach is that JS projects that include a |
||
| // NewDetectProject returns a new Runtime based on the project type of dirPath | ||
| func NewDetectProject(ctx context.Context, fs afero.Fs, dirPath string, sdkConfig hooks.SDKCLIConfig) (Runtime, error) { | ||
| var rt Runtime | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,6 +229,14 @@ func (c *ClientFactory) InitSDKConfig(ctx context.Context, dirPath string) error | |
| } | ||
| dirPath = parentDir | ||
| } | ||
| // Activate Python virtual environment if present, so hook scripts | ||
| // can resolve the venv's Python and installed packages. | ||
| if activated, err := runtime.ActivatePythonVenvIfPresent(c.Fs, c.Os, dirPath); err != nil { | ||
| c.IO.PrintDebug(ctx, "failed to activate Python virtual environment: %s", err) | ||
| } else if activated { | ||
| c.IO.PrintDebug(ctx, "Activated Python virtual environment .venv") | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I'd love to display this in the regular output, but it's tough to get a nice format because it can be displayed in many different situations.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would love to hear your thoughts on this! IIRC logic similar to what's here exists for the |
||
| } | ||
|
|
||
| configFileBytes, err := afero.ReadFile(c.Fs, hooksJSONFilePath) | ||
| if err != nil { | ||
| return err // Fixes regression: do not wrap this error, so that the caller can use `os.IsNotExists` | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️🗨️ issue(non-blocking): I'm finding the
windowsoption is erroring with perhaps unrelated hook errors in execution explored in slackapi/python-slack-hooks#96