-
Notifications
You must be signed in to change notification settings - Fork 31
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 15 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 |
|---|---|---|
|
|
@@ -15,9 +15,11 @@ | |
| package python | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| _ "embed" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
| "runtime" | ||
|
|
@@ -62,6 +64,117 @@ func (p *Python) IgnoreDirectories() []string { | |
| return []string{} | ||
| } | ||
|
|
||
| // getVenvPath returns the path to the virtual environment directory | ||
| func getVenvPath(projectDirPath string) string { | ||
| return filepath.Join(projectDirPath, ".venv") | ||
| } | ||
|
|
||
| // getPythonExecutable returns the Python executable name for the current OS | ||
| func getPythonExecutable() string { | ||
| if runtime.GOOS == "windows" { | ||
| return "python" | ||
| } | ||
| 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, 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
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. I thought about this as well. I think we want to re-activate the environment every time. While we can detect that a virtual environment is already active, we don't know if it's the virtual environment for this project or a different project. The more reliable approach is to activate it each time. Just my two-cents. |
||
|
|
||
| binDir := getVenvBinDir(venvPath) | ||
|
|
||
| if err := os.Setenv("VIRTUAL_ENV", venvPath); err != nil { | ||
| return false, err | ||
| } | ||
| if err := os.Setenv("PATH", binDir+string(filepath.ListSeparator)+os.Getenv("PATH")); err != nil { | ||
| return false, err | ||
| } | ||
| os.Unsetenv("PYTHONHOME") | ||
|
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 imagine we should create wrappers for these calls in our
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 updated this PR to use
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. @mwbrooks 🌲 So good! I think we'll want to reuse this in other places soon? |
||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| // getPipExecutable returns the path to the pip executable in the virtual environment | ||
| func getPipExecutable(venvPath string) string { | ||
| if runtime.GOOS == "windows" { | ||
| return filepath.Join(venvPath, "Scripts", "pip.exe") | ||
| } | ||
| return filepath.Join(venvPath, "bin", "pip") | ||
| } | ||
|
|
||
| // venvExists checks if a virtual environment exists at the given path | ||
| func venvExists(fs afero.Fs, venvPath string) bool { | ||
| pipPath := getPipExecutable(venvPath) | ||
| if _, err := fs.Stat(pipPath); err == nil { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // createVirtualEnvironment creates a Python virtual environment | ||
| func createVirtualEnvironment(ctx context.Context, projectDirPath string, hookExecutor hooks.HookExecutor) error { | ||
| hookScript := hooks.HookScript{ | ||
| Name: "CreateVirtualEnvironment", | ||
| Command: fmt.Sprintf("%s -m venv .venv", getPythonExecutable()), | ||
| } | ||
| stdout := bytes.Buffer{} | ||
| hookExecOpts := hooks.HookExecOpts{ | ||
| Hook: hookScript, | ||
| Stdout: &stdout, | ||
| Directory: projectDirPath, | ||
| } | ||
| _, err := hookExecutor.Execute(ctx, hookExecOpts) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create virtual environment: %w\nOutput: %s", err, stdout.String()) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // runPipInstall runs pip install with the given arguments. | ||
| // The venv does not need to be activated because pip is invoked by its full | ||
| // path inside the venv, which ensures packages are installed into the venv's | ||
| // site-packages directory. | ||
| func runPipInstall(ctx context.Context, venvPath string, projectDirPath string, hookExecutor hooks.HookExecutor, args ...string) (string, error) { | ||
| pipPath := getPipExecutable(venvPath) | ||
| cmdArgs := append([]string{pipPath, "install"}, args...) | ||
| hookScript := hooks.HookScript{ | ||
| Name: "InstallProjectDependencies", | ||
| Command: strings.Join(cmdArgs, " "), | ||
| } | ||
| stdout := bytes.Buffer{} | ||
| hookExecOpts := hooks.HookExecOpts{ | ||
| Hook: hookScript, | ||
| Stdout: &stdout, | ||
| Directory: projectDirPath, | ||
| } | ||
| _, err := hookExecutor.Execute(ctx, hookExecOpts) | ||
| output := stdout.String() | ||
| if err != nil { | ||
| return output, fmt.Errorf("pip install failed: %w", err) | ||
| } | ||
| return output, nil | ||
| } | ||
|
|
||
| // installRequirementsTxt handles adding slack-cli-hooks to requirements.txt | ||
| func installRequirementsTxt(fs afero.Fs, projectDirPath string) (output string, err error) { | ||
| requirementsFilePath := filepath.Join(projectDirPath, "requirements.txt") | ||
|
|
@@ -128,18 +241,18 @@ func installPyProjectToml(fs afero.Fs, projectDirPath string) (output string, er | |
| projectSection, exists := config["project"] | ||
| if !exists { | ||
| err := fmt.Errorf("pyproject.toml missing project section") | ||
| return fmt.Sprintf("Error: %s", err), err | ||
| return fmt.Sprintf("Error updating pyproject.toml: %s", err), err | ||
| } | ||
|
|
||
| projectMap, ok := projectSection.(map[string]interface{}) | ||
| if !ok { | ||
| err := fmt.Errorf("pyproject.toml project section is not a valid format") | ||
| return fmt.Sprintf("Error: %s", err), err | ||
| return fmt.Sprintf("Error updating pyproject.toml: %s", err), err | ||
| } | ||
|
|
||
| if _, exists := projectMap["dependencies"]; !exists { | ||
| err := fmt.Errorf("pyproject.toml missing dependencies array") | ||
| return fmt.Sprintf("Error: %s", err), err | ||
| return fmt.Sprintf("Error updating pyproject.toml: %s", err), err | ||
| } | ||
|
|
||
| // Use string manipulation to add the dependency while preserving formatting. | ||
|
|
@@ -151,7 +264,7 @@ func installPyProjectToml(fs afero.Fs, projectDirPath string) (output string, er | |
|
|
||
| if len(matches) == 0 { | ||
| err := fmt.Errorf("pyproject.toml missing dependencies array") | ||
| return fmt.Sprintf("Error: %s", err), err | ||
| return fmt.Sprintf("Error updating pyproject.toml: %s", err), err | ||
| } | ||
|
|
||
| prefix := matches[1] // "...dependencies = [" | ||
|
|
@@ -189,8 +302,7 @@ func installPyProjectToml(fs afero.Fs, projectDirPath string) (output string, er | |
| return fmt.Sprintf("Updated pyproject.toml with %s", style.Highlight(slackCLIHooksPackageSpecifier)), nil | ||
| } | ||
|
|
||
| // InstallProjectDependencies is unsupported by Python because a virtual environment is required before installing the project dependencies. | ||
| // TODO(@mbrooks) - should we confirm that the project is using Bolt Python? | ||
| // InstallProjectDependencies creates a virtual environment and installs project dependencies. | ||
| func (p *Python) InstallProjectDependencies(ctx context.Context, projectDirPath string, hookExecutor hooks.HookExecutor, ios iostreams.IOStreamer, fs afero.Fs, os types.Os) (output string, err error) { | ||
| var outputs []string | ||
| var errs []error | ||
|
|
@@ -210,44 +322,26 @@ func (p *Python) InstallProjectDependencies(ctx context.Context, projectDirPath | |
| hasPyProjectToml = true | ||
| } | ||
|
|
||
| // Defer a function to transform the return values | ||
| defer func() { | ||
| // Manual steps to setup virtual environment and install dependencies | ||
| var activateVirtualEnv = "source .venv/bin/activate" | ||
| if runtime.GOOS == "windows" { | ||
| activateVirtualEnv = `.venv\Scripts\activate` | ||
| } | ||
|
|
||
| // Get the relative path to the project directory | ||
| var projectDirPathRel, _ = getProjectDirRelPath(os, os.GetExecutionDir(), projectDirPath) | ||
|
|
||
| outputs = append(outputs, fmt.Sprintf("Manually setup a %s", style.Highlight("Python virtual environment"))) | ||
| if projectDirPathRel != "." { | ||
| outputs = append(outputs, fmt.Sprintf(" Change into the project: %s", style.CommandText(fmt.Sprintf("cd %s%s", filepath.Base(projectDirPathRel), string(filepath.Separator))))) | ||
| } | ||
| outputs = append(outputs, fmt.Sprintf(" Create virtual environment: %s", style.CommandText("python3 -m venv .venv"))) | ||
| outputs = append(outputs, fmt.Sprintf(" Activate virtual environment: %s", style.CommandText(activateVirtualEnv))) | ||
|
|
||
| // Provide appropriate install command based on which file exists | ||
| if hasRequirementsTxt { | ||
| outputs = append(outputs, fmt.Sprintf(" Install project dependencies: %s", style.CommandText("pip install -r requirements.txt"))) | ||
| } | ||
| if hasPyProjectToml { | ||
| outputs = append(outputs, fmt.Sprintf(" Install project dependencies: %s", style.CommandText("pip install -e ."))) | ||
| } | ||
| // Ensure at least one dependency file exists | ||
| if !hasRequirementsTxt && !hasPyProjectToml { | ||
| err := fmt.Errorf("no Python dependency file found (requirements.txt or pyproject.toml)") | ||
| return fmt.Sprintf("Error: %s", err), err | ||
| } | ||
|
|
||
| outputs = append(outputs, fmt.Sprintf(" Learn more: %s", style.Underline("https://docs.python.org/3/tutorial/venv.html"))) | ||
| // Get virtual environment path | ||
| venvPath := getVenvPath(projectDirPath) | ||
|
|
||
| // Get first error or nil | ||
| var firstErr error | ||
| if len(errs) > 0 { | ||
| firstErr = errs[0] | ||
| // Create virtual environment if it doesn't exist | ||
| if !venvExists(fs, venvPath) { | ||
| ios.PrintDebug(ctx, "Creating Python virtual environment") | ||
| if err := createVirtualEnvironment(ctx, projectDirPath, hookExecutor); err != nil { | ||
| outputs = append(outputs, fmt.Sprintf("Error creating virtual environment: %s", err)) | ||
| return strings.Join(outputs, "\n"), err | ||
| } | ||
|
|
||
| // Update return value | ||
| output = strings.Join(outputs, "\n") | ||
| err = firstErr | ||
| }() | ||
| outputs = append(outputs, fmt.Sprintf("Created virtual environment at %s", style.Highlight(".venv"))) | ||
| } else { | ||
| outputs = append(outputs, fmt.Sprintf("Found existing virtual environment at %s", style.Highlight(".venv"))) | ||
| } | ||
|
|
||
| // Handle requirements.txt if it exists | ||
| if hasRequirementsTxt { | ||
|
|
@@ -267,14 +361,38 @@ func (p *Python) InstallProjectDependencies(ctx context.Context, projectDirPath | |
| } | ||
| } | ||
|
|
||
| // If neither file exists, return an error | ||
| if !hasRequirementsTxt && !hasPyProjectToml { | ||
| err := fmt.Errorf("no Python dependency file found (requirements.txt or pyproject.toml)") | ||
| errs = append(errs, err) | ||
| outputs = append(outputs, fmt.Sprintf("Error: %s", err)) | ||
| // Install dependencies using pip | ||
| // When both files exist, pyproject.toml is installed first to set up the project package | ||
| // and its declared dependencies. Then requirements.txt is installed second so its version | ||
| // pins take precedence, as it typically serves as the lockfile. | ||
| if hasPyProjectToml { | ||
| ios.PrintDebug(ctx, "Installing dependencies from pyproject.toml") | ||
| pipOutput, err := runPipInstall(ctx, venvPath, projectDirPath, hookExecutor, "-e", ".") | ||
| if err != nil { | ||
| errs = append(errs, err) | ||
| outputs = append(outputs, fmt.Sprintf("Error installing from pyproject.toml: %s\n%s", err, pipOutput)) | ||
| } else { | ||
| outputs = append(outputs, fmt.Sprintf("Installed dependencies from %s", style.Highlight("pyproject.toml"))) | ||
| } | ||
| } | ||
|
|
||
| return | ||
| if hasRequirementsTxt { | ||
| ios.PrintDebug(ctx, "Installing dependencies from requirements.txt") | ||
| pipOutput, err := runPipInstall(ctx, venvPath, projectDirPath, hookExecutor, "-r", "requirements.txt") | ||
| if err != nil { | ||
| errs = append(errs, err) | ||
| outputs = append(outputs, fmt.Sprintf("Error installing from requirements.txt: %s\n%s", err, pipOutput)) | ||
| } else { | ||
| outputs = append(outputs, fmt.Sprintf("Installed dependencies from %s", style.Highlight("requirements.txt"))) | ||
| } | ||
| } | ||
|
|
||
| // Return result | ||
| output = strings.Join(outputs, "\n") | ||
| if len(errs) > 0 { | ||
| return output, errs[0] | ||
| } | ||
| return output, nil | ||
| } | ||
|
|
||
| // Name prints the name of the runtime | ||
|
|
||
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