Skip to content
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

Update agent methods to use the selected R interpreter consistently #2501

Merged
merged 29 commits into from
Dec 21, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d683678
refactor R executable into separate package for re-use, while impleme…
sagerb Dec 17, 2024
9fc2df1
Merge remote-tracking branch 'origin/main' into sagerb-extract-r-inte…
sagerb Dec 17, 2024
3b57cb7
fix linting errors
sagerb Dec 17, 2024
9be66c0
fix unit tests in CI
sagerb Dec 17, 2024
b933e77
trial fix for CI differences
sagerb Dec 17, 2024
20ecfcf
trial fix for CI differences
sagerb Dec 17, 2024
18929be
trial fix for CI differences
sagerb Dec 17, 2024
7d15f48
trial fix for CI differences
sagerb Dec 17, 2024
c66b066
trial fix for CI differences
sagerb Dec 18, 2024
811e4af
wait longer for title prompt in CI e2e test
sagerb Dec 18, 2024
78e4468
lazy load the renv lock file location (and wait to execute `renv` unt…
sagerb Dec 18, 2024
1b1ee2f
handle R executable not found - without unit tests
sagerb Dec 18, 2024
ef9d391
return CI test back to original to see if passes
sagerb Dec 18, 2024
2af2ace
remove temp debug code and adjust unit tests
sagerb Dec 18, 2024
d77b562
additional unit tests
sagerb Dec 19, 2024
4e65fff
reset CI test file
sagerb Dec 19, 2024
c56914d
remove unneeded comment
sagerb Dec 19, 2024
4b79ef2
adjust testability to use dependency injection
sagerb Dec 20, 2024
4389520
remove unneeded comments
sagerb Dec 20, 2024
cddc156
remove unneeded comments
sagerb Dec 20, 2024
7775a57
fix unit test missing mock call
sagerb Dec 20, 2024
12947da
fix unit test missing mock call
sagerb Dec 20, 2024
8964322
fix unit test missing mock call
sagerb Dec 20, 2024
5ee4fa0
fix unit test missing mock call
sagerb Dec 20, 2024
33a4886
debugging occasional vscode-ui test failure in CI
sagerb Dec 20, 2024
0feddff
debugging occasional vscode-ui test failure in CI
sagerb Dec 20, 2024
a28a3d1
debugging occasional vscode-ui test failure in CI
sagerb Dec 20, 2024
ed931fb
debugging occasional vscode-ui test failure in CI
sagerb Dec 20, 2024
818652c
debugging occasional vscode-ui test failure in CI
sagerb Dec 21, 2024
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
45 changes: 19 additions & 26 deletions internal/initialize/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,17 +59,25 @@ func inspectProject(base util.AbsolutePath, python util.Path, rExecutable util.P
}
cfg.Python = pyConfig
}
needR, err := requiresR(cfg, base)

rInspector, err := RInspectorFactory(base, rExecutable, log)
if err != nil {
return nil, err
}

needR, err := rInspector.RequiresR(cfg)
if err != nil {
log.Debug("Error while determining R as a requirement", "error", err.Error())
return nil, err
}
if needR {
inspector := RInspectorFactory(base, rExecutable, log)
rConfig, err := inspector.InspectR()
rConfig, err := rInspector.InspectR()
if err != nil {
log.Debug("Error while inspecting to generate an R based configuration", "error", err.Error())
return nil, err
}
cfg.R = rConfig
cfg.Files = append(cfg.Files, fmt.Sprint("/", cfg.R.PackageFile))
}
cfg.Comments = strings.Split(initialComment, "\n")

Expand All @@ -92,26 +100,6 @@ func requiresPython(cfg *config.Config, base util.AbsolutePath) (bool, error) {
return exists, nil
}

func requiresR(cfg *config.Config, base util.AbsolutePath) (bool, error) {
if cfg.R != nil {
// InferType returned an R configuration for us to fill in.
return true, nil
}
if cfg.Type != config.ContentTypeHTML && !cfg.Type.IsPythonContent() {
// Presence of renv.lock implies R is needed,
// unless we're deploying pre-rendered Rmd or Quarto
// (where there will usually be a source file and
// associated lockfile in the directory)
lockfilePath := base.Join(inspect.DefaultRenvLockfile)
exists, err := lockfilePath.Exists()
if err != nil {
return false, err
}
return exists, nil
}
return false, nil
}

func normalizeConfig(
cfg *config.Config,
base util.AbsolutePath,
Expand Down Expand Up @@ -161,14 +149,19 @@ func normalizeConfig(
cfg.Python = pyConfig
cfg.Files = append(cfg.Files, fmt.Sprint("/", cfg.Python.PackageFile))
}
needR, err := requiresR(cfg, base)

rInspector, err := RInspectorFactory(base, rExecutable, log)
if err != nil {
log.Debug("Error while creating the RInspector", "error", err.Error())
return err
}
needR, err := rInspector.RequiresR(cfg)
if err != nil {
log.Debug("Error while determining R as a requirement", "error", err.Error())
return err
}
if needR {
inspector := RInspectorFactory(base, rExecutable, log)
rConfig, err := inspector.InspectR()
rConfig, err := rInspector.InspectR()
if err != nil {
log.Debug("Error while inspecting to generate an R based configuration", "error", err.Error())
return err
Expand Down
122 changes: 109 additions & 13 deletions internal/initialize/initialize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ import (
"github.com/posit-dev/publisher/internal/config"
"github.com/posit-dev/publisher/internal/inspect"
"github.com/posit-dev/publisher/internal/inspect/detectors"
"github.com/posit-dev/publisher/internal/interpreters"
"github.com/posit-dev/publisher/internal/logging"
"github.com/posit-dev/publisher/internal/util"
"github.com/posit-dev/publisher/internal/util/utiltest"
"github.com/spf13/afero"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/suite"
)

// TODO = initialize not currently testing R project

type InitializeSuite struct {
utiltest.Suite
cwd util.AbsolutePath
Expand All @@ -34,6 +38,14 @@ func (s *InitializeSuite) SetupTest() {
s.cwd = cwd
err = cwd.MkdirAll(0700)
s.NoError(err)

interpreters.NewRInterpreter = func(baseDir util.AbsolutePath, rExec util.Path, log logging.Logger) interpreters.RInterpreter {
i := interpreters.NewMockRInterpreter()
i.On("Init").Return(nil)
i.On("RequiresR", mock.Anything).Return(false, nil)
i.On("GetLockFilePath").Return(util.RelativePath{}, false, nil)
return i
}
}

func (s *InitializeSuite) TestInitEmpty() {
Expand All @@ -49,14 +61,63 @@ func (s *InitializeSuite) TestInitEmpty() {
s.Equal("My App", cfg.Title)
}

func (s *InitializeSuite) createAppPy() {
func (s *InitializeSuite) createAppPy() util.AbsolutePath {
appPath := s.cwd.Join("app.py")
err := appPath.WriteFile([]byte(`
from flask import Flask
app = Flask(__name__)
app.run()
`), 0666)
s.NoError(err)
return appPath
}

func (s *InitializeSuite) createAppR() util.AbsolutePath {
appPath := s.cwd.Join("app.R")
err := appPath.WriteFile([]byte(`
library(shiny)

# Define UI for application that draws a histogram
ui <- fluidPage(

# Application title
titlePanel("Old Faithful Geyser Data"),

# Sidebar with a slider input for number of bins
sidebarLayout(
sidebarPanel(
sliderInput("bins",
"Number of bins:",
min = 1,
max = 50,
value = 30)
),

# Show a plot of the generated distribution
mainPanel(
plotOutput("distPlot")
)
)
)

# Define server logic required to draw a histogram
server <- function(input, output) {

output$distPlot <- renderPlot({
# generate bins based on input$bins from ui.R
x <- faithful[, 2]
bins <- seq(min(x), max(x), length.out = input$bins + 1)

# draw the histogram with the specified number of bins
hist(x, breaks = bins, col = 'darkgray', border = 'white')
})
}

# Run the application
shinyApp(ui = ui, server = server)
`), 0666)
s.NoError(err)
return appPath
}

func (s *InitializeSuite) createHTML() {
Expand Down Expand Up @@ -88,10 +149,26 @@ func makeMockPythonInspector(util.AbsolutePath, util.Path, logging.Logger) inspe
return pyInspector
}

var expectedRConfig = &config.R{
Version: "1.2.3",
PackageManager: "renv",
PackageFile: "renv.lock",
}

func setupMockRInspector(requiredRReturnValue bool, requiredRError error) func(util.AbsolutePath, util.Path, logging.Logger) (inspect.RInspector, error) {
return func(util.AbsolutePath, util.Path, logging.Logger) (inspect.RInspector, error) {
rInspector := inspect.NewMockRInspector()
rInspector.On("InspectR").Return(expectedRConfig, nil)
rInspector.On("RequiresR").Return(requiredRReturnValue, requiredRError)
return rInspector, nil
}
}

func (s *InitializeSuite) TestInitInferredType() {
log := logging.New()
s.createAppPy()
PythonInspectorFactory = makeMockPythonInspector
RInspectorFactory = setupMockRInspector(false, nil)
configName := ""
cfg, err := Init(s.cwd, configName, util.Path{}, util.Path{}, log)
s.NoError(err)
Expand All @@ -108,6 +185,7 @@ func (s *InitializeSuite) TestInitRequirementsFile() {
s.createHTML()
s.createRequirementsFile()
PythonInspectorFactory = makeMockPythonInspector
RInspectorFactory = setupMockRInspector(false, nil)
configName := ""
cfg, err := Init(s.cwd, configName, util.Path{}, util.Path{}, log)
s.NoError(err)
Expand All @@ -123,6 +201,7 @@ func (s *InitializeSuite) TestInitIfNeededWhenNeeded() {
log := logging.New()
s.createAppPy()
PythonInspectorFactory = makeMockPythonInspector
RInspectorFactory = setupMockRInspector(false, nil)
configName := ""
err := InitIfNeeded(s.cwd, configName, log)
s.NoError(err)
Expand Down Expand Up @@ -150,6 +229,9 @@ func (s *InitializeSuite) TestInitIfNeededWhenNotNeeded() {
PythonInspectorFactory = func(util.AbsolutePath, util.Path, logging.Logger) inspect.PythonInspector {
return &inspect.MockPythonInspector{}
}
RInspectorFactory = func(util.AbsolutePath, util.Path, logging.Logger) (inspect.RInspector, error) {
return &inspect.MockRInspector{}, nil
}
err := InitIfNeeded(s.cwd, configName, log)
s.NoError(err)
newConfig, err := config.FromFile(configPath)
Expand All @@ -159,25 +241,39 @@ func (s *InitializeSuite) TestInitIfNeededWhenNotNeeded() {

func (s *InitializeSuite) TestGetPossibleConfigs() {
log := logging.New()
s.createAppPy()
appPy := s.createAppPy()
exist, err := appPy.Exists()
s.NoError(err)
s.Equal(true, exist)
appR := s.createAppR()
exist, err = appR.Exists()
s.NoError(err)
s.Equal(true, exist)

err := s.cwd.Join("index.html").WriteFile([]byte(`<html></html>`), 0666)
err = s.cwd.Join("index.html").WriteFile([]byte(`<html></html>`), 0666)
s.NoError(err)

PythonInspectorFactory = makeMockPythonInspector
RInspectorFactory = setupMockRInspector(true, nil)

configs, err := GetPossibleConfigs(s.cwd, util.Path{}, util.Path{}, util.RelativePath{}, log)
s.NoError(err)

s.Len(configs, 2)
s.Equal(config.ContentTypePythonFlask, configs[0].Type)
s.Equal("app.py", configs[0].Entrypoint)
s.Equal([]string{"/app.py", "/requirements.txt"}, configs[0].Files)
s.Equal(expectedPyConfig, configs[0].Python)

s.Equal(config.ContentTypeHTML, configs[1].Type)
s.Equal("index.html", configs[1].Entrypoint)
s.Equal([]string{"/index.html"}, configs[1].Files)
s.Nil(configs[1].Python)
s.Len(configs, 3)
s.Equal(config.ContentTypeRShiny, configs[0].Type)
s.Equal("app.R", configs[0].Entrypoint)
s.Equal([]string{"/app.R", "/renv.lock"}, configs[0].Files)
s.Equal(expectedRConfig, configs[0].R)

s.Equal(config.ContentTypePythonFlask, configs[1].Type)
s.Equal("app.py", configs[1].Entrypoint)
s.Equal([]string{"/app.py", "/requirements.txt", "/renv.lock"}, configs[1].Files)
s.Equal(expectedPyConfig, configs[1].Python)

s.Equal(config.ContentTypeHTML, configs[2].Type)
s.Equal("index.html", configs[2].Entrypoint)
s.Equal([]string{"/index.html", "/renv.lock"}, configs[2].Files)
s.Nil(configs[2].Python)
}

func (s *InitializeSuite) TestGetPossibleConfigsEmpty() {
Expand Down
46 changes: 32 additions & 14 deletions internal/inspect/dependencies/renv/available_packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"github.com/posit-dev/publisher/internal/executor"
"github.com/posit-dev/publisher/internal/interpreters"
"github.com/posit-dev/publisher/internal/logging"
"github.com/posit-dev/publisher/internal/util"
)
Expand All @@ -24,20 +25,21 @@ type AvailablePackagesLister interface {
}

type defaultAvailablePackagesLister struct {
base util.AbsolutePath
rExecutable util.Path
rExecutor executor.Executor
base util.AbsolutePath
rInterpreter interpreters.RInterpreter
rExecutor executor.Executor
}

func NewAvailablePackageLister(base util.AbsolutePath, rExecutable util.Path) *defaultAvailablePackagesLister {
if rExecutable.String() == "" {
rExecutable = util.NewPath("R", nil)
}
func NewAvailablePackageLister(base util.AbsolutePath, rExecutable util.Path, log logging.Logger) (*defaultAvailablePackagesLister, error) {

rInterpreter := interpreters.NewRInterpreter(base, rExecutable, log)
err := rInterpreter.Init()

return &defaultAvailablePackagesLister{
base: base,
rExecutable: rExecutable,
rExecutor: executor.NewExecutor(),
}
base: base,
rInterpreter: rInterpreter,
rExecutor: executor.NewExecutor(),
}, err
}

func repoUrlsAsStrings(repos []Repository) string {
Expand Down Expand Up @@ -70,8 +72,13 @@ func (l *defaultAvailablePackagesLister) ListAvailablePackages(repos []Repositor
repoNames := repoNamesAsStrings(repos)
packageListCode := fmt.Sprintf(packageListCodeTemplate, repoUrls, repoNames)

rExecutable, err := l.rInterpreter.GetRExecutable()
if err != nil {
return nil, err
}

out, _, err := l.rExecutor.RunCommand(
l.rExecutable.String(),
rExecutable.String(),
[]string{
"-s",
"-e",
Expand Down Expand Up @@ -109,8 +116,13 @@ func (l *defaultAvailablePackagesLister) GetBioconductorRepos(base util.Absolute
escapedBase := strings.ReplaceAll(l.base.String(), `\`, `\\`)
biocRepoListCode := fmt.Sprintf(bioconductorReposCodeTemplate, escapedBase)

rExecutable, err := l.rInterpreter.GetRExecutable()
if err != nil {
return nil, err
}

out, _, err := l.rExecutor.RunCommand(
l.rExecutable.String(),
rExecutable.String(),
[]string{
"-s",
"-e",
Expand Down Expand Up @@ -144,9 +156,15 @@ func (l *defaultAvailablePackagesLister) GetBioconductorRepos(base util.Absolute
}

func (l *defaultAvailablePackagesLister) GetLibPaths(log logging.Logger) ([]util.AbsolutePath, error) {

rExecutable, err := l.rInterpreter.GetRExecutable()
if err != nil {
return nil, err
}

const getLibPathsCode = `cat(.libPaths(), sep="\n")`
out, _, err := l.rExecutor.RunCommand(
l.rExecutable.String(),
rExecutable.String(),
[]string{
"-s",
"-e",
Expand Down
Loading
Loading