Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions internal/runtime/python/python.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

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" {
Expand Down
61 changes: 61 additions & 0 deletions internal/runtime/python/python_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/json"
"fmt"
"path/filepath"
"runtime"
"testing"

"github.com/slackapi/slack-cli/internal/config"
Expand Down Expand Up @@ -314,6 +315,66 @@ func Test_venvExists(t *testing.T) {
}
}

func Test_getVenvBinDir(t *testing.T) {
result := getVenvBinDir("/path/to/.venv")
if runtime.GOOS == "windows" {
require.Equal(t, filepath.Join("/path/to/.venv", "Scripts"), result)
} else {
require.Equal(t, filepath.Join("/path/to/.venv", "bin"), result)
}
}

func Test_ActivateVenvIfPresent(t *testing.T) {
tests := map[string]struct {
createVenv bool
expectedActivated bool
}{
"activates venv when it exists": {
createVenv: true,
expectedActivated: true,
},
"no-op when venv does not exist": {
createVenv: false,
expectedActivated: false,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
fs := slackdeps.NewFsMock()
osMock := slackdeps.NewOsMock()
projectDir := "/path/to/project"
venvPath := filepath.Join(projectDir, ".venv")

originalPath := "/usr/bin:/bin"
osMock.On("Getenv", "PATH").Return(originalPath)
osMock.AddDefaultMocks()

if tc.createVenv {
// Create the pip executable so venvExists returns true
pipPath := getPipExecutable(venvPath)
err := fs.MkdirAll(filepath.Dir(pipPath), 0755)
require.NoError(t, err)
err = afero.WriteFile(fs, pipPath, []byte(""), 0755)
require.NoError(t, err)
}

activated, err := ActivateVenvIfPresent(fs, osMock, projectDir)
require.NoError(t, err)
require.Equal(t, tc.expectedActivated, activated)

if tc.expectedActivated {
expectedBinDir := getVenvBinDir(venvPath)
osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
osMock.AssertCalled(t, "Setenv", "PATH", expectedBinDir+string(filepath.ListSeparator)+originalPath)
osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
} else {
osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
}
})
}
}

func Test_Python_InstallProjectDependencies(t *testing.T) {
tests := map[string]struct {
existingFiles map[string]string
Expand Down
8 changes: 8 additions & 0 deletions internal/runtime/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: While it feels wrong to have Python in the generic runtime this allows us to activate it for any project. We could move this into the Python runtime and think of a more general function that could be used across all runtimes.

Upside of this approach is that JS projects that include a .venv (perhaps for scripts) can be activated automatically.

// 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
Expand Down
52 changes: 52 additions & 0 deletions internal/runtime/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@
package runtime

import (
"path/filepath"
"testing"

"github.com/slackapi/slack-cli/internal/hooks"
"github.com/slackapi/slack-cli/internal/runtime/deno"
"github.com/slackapi/slack-cli/internal/runtime/node"
"github.com/slackapi/slack-cli/internal/runtime/python"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/spf13/afero"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -57,6 +60,55 @@ func Test_Runtime_New(t *testing.T) {
}
}

func Test_ActivatePythonVenvIfPresent(t *testing.T) {
tests := map[string]struct {
createVenv bool
expectedActivated bool
}{
"activates venv when it exists": {
createVenv: true,
expectedActivated: true,
},
"no-op when venv does not exist": {
createVenv: false,
expectedActivated: false,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
fs := slackdeps.NewFsMock()
osMock := slackdeps.NewOsMock()
projectDir := "/path/to/project"
venvPath := filepath.Join(projectDir, ".venv")

osMock.On("Getenv", "PATH").Return("/usr/bin:/bin")
osMock.AddDefaultMocks()

if tc.createVenv {
// Create the pip executable so venvExists returns true
pipPath := filepath.Join(venvPath, "bin", "pip")
err := fs.MkdirAll(filepath.Dir(pipPath), 0755)
require.NoError(t, err)
err = afero.WriteFile(fs, pipPath, []byte(""), 0755)
require.NoError(t, err)
}

activated, err := ActivatePythonVenvIfPresent(fs, osMock, projectDir)
require.NoError(t, err)
require.Equal(t, tc.expectedActivated, activated)

if tc.expectedActivated {
osMock.AssertCalled(t, "Setenv", "VIRTUAL_ENV", venvPath)
osMock.AssertCalled(t, "Setenv", "PATH", mock.Anything)
osMock.AssertCalled(t, "Unsetenv", "PYTHONHOME")
} else {
osMock.AssertNotCalled(t, "Setenv", mock.Anything, mock.Anything)
osMock.AssertNotCalled(t, "Unsetenv", mock.Anything)
}
})
}
}

func Test_Runtime_NewDetectProject(t *testing.T) {
tests := map[string]struct {
sdkConfig hooks.SDKCLIConfig
Expand Down
8 changes: 8 additions & 0 deletions internal/shared/clients.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,14 @@ func (c *ClientFactory) InitSDKConfig(ctx context.Context, dirPath string) error
// Move upward one directory level
dirPath = filepath.Dir(dirPath)
}
// 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")
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

}

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`
Expand Down
3 changes: 3 additions & 0 deletions internal/shared/types/slackdeps.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ type Os interface {
// Setenv defaults to `os.Setenv` and can be mocked to test
Setenv(key string, value string) error

// Unsetenv defaults to `os.Unsetenv` and can be mocked to test
Unsetenv(key string) error

// Getwd defaults to `os.Getwd` and can be mocked to test
Getwd() (dir string, err error)

Expand Down
5 changes: 5 additions & 0 deletions internal/slackdeps/os.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ func (c *Os) Setenv(key string, value string) error {
return os.Setenv(key, value)
}

// Unsetenv defaults to `os.Unsetenv` and can be mocked to test
func (c *Os) Unsetenv(key string) error {
return os.Unsetenv(key)
}

// Getwd defaults to `os.Getwd` and can be mocked to test
func (c *Os) Getwd() (dir string, err error) {
return os.Getwd()
Expand Down
9 changes: 9 additions & 0 deletions internal/slackdeps/os_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func (m *OsMock) AddDefaultMocks() {
m.On("LookPath", mock.Anything).Return("", nil)
m.On("LookupEnv", mock.Anything).Return("", false)
m.On("Setenv", mock.Anything, mock.Anything).Return(nil)
m.On("Unsetenv", mock.Anything).Return(nil)
m.On("Getwd").Return(MockWorkingDirectory, nil)
m.On("UserHomeDir").Return(MockHomeDirectory, nil)
m.On("GetExecutionDir").Return(MockHomeDirectory, nil)
Expand Down Expand Up @@ -83,6 +84,14 @@ func (m *OsMock) Setenv(key string, value string) error {
return args.Error(0)
}

// Unsetenv mocks the unsetting of an environment variable
func (m *OsMock) Unsetenv(key string) error {
m.On("Getenv", key).Return("")
m.On("LookupEnv", key).Return("", false)
args := m.Called(key)
return args.Error(0)
}

// Getwd mocks returning the working directory.
func (m *OsMock) Getwd() (_dir string, _err error) {
args := m.Called()
Expand Down
Loading