Skip to content

Commit 4f51fa6

Browse files
mwbrookszimegClaude
authored
feat(python): auto-activate venv before hook execution (#347)
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com> Co-authored-by: Claude <svc-devxp-claude@slack-corp.com>
1 parent 3417188 commit 4f51fa6

File tree

8 files changed

+184
-0
lines changed

8 files changed

+184
-0
lines changed

internal/runtime/python/python.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,45 @@ func getPythonExecutable() string {
7676
return "python3"
7777
}
7878

79+
// getVenvBinDir returns the platform-specific bin directory inside a virtual environment
80+
func getVenvBinDir(venvPath string) string {
81+
if runtime.GOOS == "windows" {
82+
return filepath.Join(venvPath, "Scripts")
83+
}
84+
return filepath.Join(venvPath, "bin")
85+
}
86+
87+
// ActivateVenvIfPresent sets the process environment variables that a Python
88+
// virtual environment's activate script would set, so that child processes
89+
// (hook scripts) inherit the activated venv. This is a no-op when no .venv
90+
// directory exists in projectDir.
91+
//
92+
// The three env vars (VIRTUAL_ENV, PATH, PYTHONHOME) are the stable contract
93+
// defined by PEP 405 (Python 3.3, 2012). Other tools (Poetry, tox, pipx) use
94+
// the same approach. Sourcing the shell-specific activate script (activate,
95+
// activate.fish, Activate.ps1) would be higher maintenance because it varies
96+
// by shell.
97+
func ActivateVenvIfPresent(fs afero.Fs, osClient types.Os, projectDir string) (bool, error) {
98+
venvPath := getVenvPath(projectDir)
99+
if !venvExists(fs, venvPath) {
100+
return false, nil
101+
}
102+
103+
binDir := getVenvBinDir(venvPath)
104+
105+
if err := osClient.Setenv("VIRTUAL_ENV", venvPath); err != nil {
106+
return false, err
107+
}
108+
if err := osClient.Setenv("PATH", binDir+string(filepath.ListSeparator)+osClient.Getenv("PATH")); err != nil {
109+
return false, err
110+
}
111+
if err := osClient.Unsetenv("PYTHONHOME"); err != nil {
112+
return false, err
113+
}
114+
115+
return true, nil
116+
}
117+
79118
// getPipExecutable returns the path to the pip executable in the virtual environment
80119
func getPipExecutable(venvPath string) string {
81120
if runtime.GOOS == "windows" {

internal/runtime/python/python_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,66 @@ func Test_venvExists(t *testing.T) {
296296
}
297297
}
298298

299+
func Test_getVenvBinDir(t *testing.T) {
300+
result := getVenvBinDir("/path/to/.venv")
301+
if runtime.GOOS == "windows" {
302+
require.Equal(t, filepath.Join("/path/to/.venv", "Scripts"), result)
303+
} else {
304+
require.Equal(t, filepath.Join("/path/to/.venv", "bin"), result)
305+
}
306+
}
307+
308+
func Test_ActivateVenvIfPresent(t *testing.T) {
309+
tests := map[string]struct {
310+
createVenv bool
311+
expectedActivated bool
312+
}{
313+
"activates venv when it exists": {
314+
createVenv: true,
315+
expectedActivated: true,
316+
},
317+
"no-op when venv does not exist": {
318+
createVenv: false,
319+
expectedActivated: false,
320+
},
321+
}
322+
for name, tc := range tests {
323+
t.Run(name, func(t *testing.T) {
324+
fs := slackdeps.NewFsMock()
325+
osMock := slackdeps.NewOsMock()
326+
projectDir := "/path/to/project"
327+
venvPath := filepath.Join(projectDir, ".venv")
328+
329+
originalPath := "/usr/bin:/bin"
330+
osMock.On("Getenv", "PATH").Return(originalPath)
331+
osMock.AddDefaultMocks()
332+
333+
if tc.createVenv {
334+
// Create the pip executable so venvExists returns true
335+
pipPath := getPipExecutable(venvPath)
336+
err := fs.MkdirAll(filepath.Dir(pipPath), 0755)
337+
require.NoError(t, err)
338+
err = afero.WriteFile(fs, pipPath, []byte(""), 0755)
339+
require.NoError(t, err)
340+
}
341+
342+
activated, err := ActivateVenvIfPresent(fs, osMock, projectDir)
343+
require.NoError(t, err)
344+
require.Equal(t, tc.expectedActivated, activated)
345+
346+
if tc.expectedActivated {
347+
expectedBinDir := getVenvBinDir(venvPath)
348+
osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
349+
osMock.AssertCalled(t, "Setenv", "PATH", expectedBinDir+string(filepath.ListSeparator)+originalPath)
350+
osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
351+
} else {
352+
osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
353+
osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
354+
}
355+
})
356+
}
357+
}
358+
299359
func Test_Python_InstallProjectDependencies(t *testing.T) {
300360
tests := map[string]struct {
301361
existingFiles map[string]string

internal/runtime/runtime.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,14 @@ func New(runtimeName string) (Runtime, error) {
6666
return rt, nil
6767
}
6868

69+
// ActivatePythonVenvIfPresent activates a Python virtual environment if one
70+
// exists in the given project directory. This sets VIRTUAL_ENV, prepends the
71+
// venv bin directory to PATH, and unsets PYTHONHOME on the current process so
72+
// that child processes (hook scripts) inherit the activated venv.
73+
func ActivatePythonVenvIfPresent(fs afero.Fs, os types.Os, projectDir string) (bool, error) {
74+
return python.ActivateVenvIfPresent(fs, os, projectDir)
75+
}
76+
6977
// NewDetectProject returns a new Runtime based on the project type of dirPath
7078
func NewDetectProject(ctx context.Context, fs afero.Fs, dirPath string, sdkConfig hooks.SDKCLIConfig) (Runtime, error) {
7179
var rt Runtime

internal/runtime/runtime_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,17 @@
1515
package runtime
1616

1717
import (
18+
"path/filepath"
1819
"testing"
1920

2021
"github.com/slackapi/slack-cli/internal/hooks"
2122
"github.com/slackapi/slack-cli/internal/runtime/deno"
2223
"github.com/slackapi/slack-cli/internal/runtime/node"
2324
"github.com/slackapi/slack-cli/internal/runtime/python"
2425
"github.com/slackapi/slack-cli/internal/slackcontext"
26+
"github.com/slackapi/slack-cli/internal/slackdeps"
2527
"github.com/spf13/afero"
28+
"github.com/stretchr/testify/mock"
2629
"github.com/stretchr/testify/require"
2730
)
2831

@@ -57,6 +60,55 @@ func Test_Runtime_New(t *testing.T) {
5760
}
5861
}
5962

63+
func Test_ActivatePythonVenvIfPresent(t *testing.T) {
64+
tests := map[string]struct {
65+
createVenv bool
66+
expectedActivated bool
67+
}{
68+
"activates venv when it exists": {
69+
createVenv: true,
70+
expectedActivated: true,
71+
},
72+
"no-op when venv does not exist": {
73+
createVenv: false,
74+
expectedActivated: false,
75+
},
76+
}
77+
for name, tc := range tests {
78+
t.Run(name, func(t *testing.T) {
79+
fs := slackdeps.NewFsMock()
80+
osMock := slackdeps.NewOsMock()
81+
projectDir := "/path/to/project"
82+
venvPath := filepath.Join(projectDir, ".venv")
83+
84+
osMock.On("Getenv", "PATH").Return("/usr/bin:/bin")
85+
osMock.AddDefaultMocks()
86+
87+
if tc.createVenv {
88+
// Create the pip executable so venvExists returns true
89+
pipPath := filepath.Join(venvPath, "bin", "pip")
90+
err := fs.MkdirAll(filepath.Dir(pipPath), 0755)
91+
require.NoError(t, err)
92+
err = afero.WriteFile(fs, pipPath, []byte(""), 0755)
93+
require.NoError(t, err)
94+
}
95+
96+
activated, err := ActivatePythonVenvIfPresent(fs, osMock, projectDir)
97+
require.NoError(t, err)
98+
require.Equal(t, tc.expectedActivated, activated)
99+
100+
if tc.expectedActivated {
101+
osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
102+
osMock.AssertCalled(t, "Setenv", "PATH", mock.Anything)
103+
osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
104+
} else {
105+
osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
106+
osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
107+
}
108+
})
109+
}
110+
}
111+
60112
func Test_Runtime_NewDetectProject(t *testing.T) {
61113
tests := map[string]struct {
62114
sdkConfig hooks.SDKCLIConfig

internal/shared/clients.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,14 @@ func (c *ClientFactory) InitSDKConfig(ctx context.Context, dirPath string) error
229229
}
230230
dirPath = parentDir
231231
}
232+
// Activate Python virtual environment if present, so hook scripts
233+
// can resolve the venv's Python and installed packages.
234+
if activated, err := runtime.ActivatePythonVenvIfPresent(c.Fs, c.Os, dirPath); err != nil {
235+
c.IO.PrintDebug(ctx, "failed to activate Python virtual environment: %s", err)
236+
} else if activated {
237+
c.IO.PrintDebug(ctx, "Activated Python virtual environment .venv")
238+
}
239+
232240
configFileBytes, err := afero.ReadFile(c.Fs, hooksJSONFilePath)
233241
if err != nil {
234242
return err // Fixes regression: do not wrap this error, so that the caller can use `os.IsNotExists`

internal/shared/types/slackdeps.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ type Os interface {
3838
// Setenv defaults to `os.Setenv` and can be mocked to test
3939
Setenv(key string, value string) error
4040

41+
// Unsetenv defaults to `os.Unsetenv` and can be mocked to test
42+
Unsetenv(key string) error
43+
4144
// Getwd defaults to `os.Getwd` and can be mocked to test
4245
Getwd() (dir string, err error)
4346

internal/slackdeps/os.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,11 @@ func (c *Os) Setenv(key string, value string) error {
5353
return os.Setenv(key, value)
5454
}
5555

56+
// Unsetenv defaults to `os.Unsetenv` and can be mocked to test
57+
func (c *Os) Unsetenv(key string) error {
58+
return os.Unsetenv(key)
59+
}
60+
5661
// Getwd defaults to `os.Getwd` and can be mocked to test
5762
func (c *Os) Getwd() (dir string, err error) {
5863
return os.Getwd()

internal/slackdeps/os_mock.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ func (m *OsMock) AddDefaultMocks() {
4646
m.On("LookPath", mock.Anything).Return("", nil)
4747
m.On("LookupEnv", mock.Anything).Return("", false)
4848
m.On("Setenv", mock.Anything, mock.Anything).Return(nil)
49+
m.On("Unsetenv", mock.Anything).Return(nil)
4950
m.On("Getwd").Return(MockWorkingDirectory, nil)
5051
m.On("UserHomeDir").Return(MockHomeDirectory, nil)
5152
m.On("GetExecutionDir").Return(MockHomeDirectory, nil)
@@ -83,6 +84,14 @@ func (m *OsMock) Setenv(key string, value string) error {
8384
return args.Error(0)
8485
}
8586

87+
// Unsetenv mocks the unsetting of an environment variable
88+
func (m *OsMock) Unsetenv(key string) error {
89+
m.On("Getenv", key).Return("")
90+
m.On("LookupEnv", key).Return("", false)
91+
args := m.Called(key)
92+
return args.Error(0)
93+
}
94+
8695
// Getwd mocks returning the working directory.
8796
func (m *OsMock) Getwd() (_dir string, _err error) {
8897
args := m.Called()

0 commit comments

Comments
 (0)