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

Revise quarto argument #828

Merged
merged 8 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# rsconnect (development version)

* `deployApp()`'s `quarto` argument now takes values `TRUE`, `FALSE` or
hadley marked this conversation as resolved.
Show resolved Hide resolved
`NA`. The previous value (a path to a quarto binary) is now deprecated (#658).

* `deployApp()` gains a new `envVars` argument which takes a vector of the
names of environment variables that should be securely copied to the server.
The names (not values) of these environment variables are also saved in the
Expand Down
117 changes: 36 additions & 81 deletions R/appMetadata.R
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
appMetadata <- function(appDir,
appFiles = NULL,
appPrimaryDoc = NULL,
quarto = NULL,
usesQuarto = NA,
contentCategory = NULL,
isCloudServer = FALSE,
metadata = list()) {

check_bool(usesQuarto, allow_na = TRUE)

appFiles <- listDeploymentFiles(appDir, appFiles)
checkAppLayout(appDir, appPrimaryDoc)

# User has supplied quarto path or quarto package/IDE has supplied metadata
# If quarto package/IDE has supplied metadata, always use quarto
# https://github.com/quarto-dev/quarto-r/blob/08caf0f42504e7/R/publish.R#L117-L121
# https://github.com/rstudio/rstudio/blob/3d45a20307f650/src/cpp/session/modules/SessionRSConnect.cpp#L81-L123
hasQuarto <- !is.null(quarto) || hasQuartoMetadata(metadata)
if (hasQuartoMetadata(metadata)) {
usesQuarto <- TRUE
}

# Generally we want to infer appPrimaryDoc from appMode, but there's one
# special case
Expand All @@ -24,7 +28,7 @@ appMetadata <- function(appDir,
rootFiles <- appFiles[dirname(appFiles) == "."]
appMode <- inferAppMode(
file.path(appDir, appFiles),
hasQuarto = hasQuarto,
usesQuarto = usesQuarto,
isCloudServer = isCloudServer
)
}
Expand All @@ -44,12 +48,16 @@ appMetadata <- function(appDir,
appDir = appDir,
files = appFiles
)
quartoInfo <- inferQuartoInfo(
appDir = appDir,
appPrimaryDoc = appPrimaryDoc,
quarto = quarto,
metadata = metadata
)

if (appIsQuartoDocument(appMode)) {
quartoInfo <- inferQuartoInfo(
metadata = metadata,
appDir = appDir,
appPrimaryDoc = appPrimaryDoc
)
} else {
quartoInfo <- NULL
}

list(
appMode = appMode,
Expand Down Expand Up @@ -102,7 +110,7 @@ checkAppLayout <- function(appDir, appPrimaryDoc = NULL) {

# infer the mode of the application from files in the root dir
inferAppMode <- function(absoluteAppFiles,
hasQuarto = FALSE,
usesQuarto = FALSE,
hadley marked this conversation as resolved.
Show resolved Hide resolved
isCloudServer = FALSE) {

matchingNames <- function(paths, pattern) {
Expand All @@ -125,22 +133,13 @@ inferAppMode <- function(absoluteAppFiles,
rmdFiles <- matchingNames(absoluteAppFiles, "\\.rmd$")
qmdFiles <- matchingNames(absoluteAppFiles, "\\.qmd$")

# We make Quarto requirement conditional on the presence of files that Quarto
# can render and _quarto.yml, because keying off the presence of qmds
# *or* _quarto.yml was causing deployment failures in static content.
# https://github.com/rstudio/rstudio/issues/11444
quartoYml <- matchingNames(absoluteAppFiles, "^_quarto.y(a)?ml$")
hasQuartoYaml <- length(quartoYml) > 0
hasQuartoCompatibleFiles <- length(qmdFiles) > 0 || length(rmdFiles > 0)
requiresQuarto <- (hasQuartoCompatibleFiles && hasQuartoYaml) || length(qmdFiles) > 0

# We gate the deployment of content that appears to be Quarto behind the
# presence of Quarto metadata. Rmd files can still be deployed as Quarto
if (requiresQuarto && !hasQuarto) {
cli::cli_abort(c(
"Can't deploy Quarto content when {.arg quarto} is {.code NULL}.",
i = "Please supply a path to a quarto binary in {.arg quarto}."
))
if (is.na(usesQuarto)) {
# Can't use _quarto.yml alone because it causes deployment failures for
# static content: https://github.com/rstudio/rstudio/issues/11444
quartoYml <- matchingNames(absoluteAppFiles, "^_quarto.y(a)?ml$")

usesQuarto <- length(qmdFiles) > 0 ||
(length(quartoYml) > 0 && length(rmdFiles > 0))
}

# Documents with "server: shiny" in their YAML front matter need shiny too
Expand All @@ -150,7 +149,7 @@ inferAppMode <- function(absoluteAppFiles,
if (hasShinyQmd) {
return("quarto-shiny")
} else if (hasShinyRmd) {
if (hasQuarto) {
if (usesQuarto) {
return("quarto-shiny")
} else {
return("rmd-shiny")
Expand All @@ -168,7 +167,7 @@ inferAppMode <- function(absoluteAppFiles,
# Any non-Shiny R Markdown or Quarto documents are rendered content and get
# rmd-static or quarto-static.
if (length(rmdFiles) > 0 || length(qmdFiles) > 0) {
if (hasQuarto) {
if (usesQuarto) {
return("quarto-static")
} else {
# For Shinyapps and posit.cloud, treat "rmd-static" app mode as "rmd-shiny" so that
Expand Down Expand Up @@ -275,6 +274,14 @@ appIsDocument <- function(appMode) {
)
}

appIsQuartoDocument <- function(appMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Because this might be a single document or a more complicated project; maybe appIsQuartoContent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Words are hard, eh? Other thoughts: isQuartoContent, appModeIsQuarto, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just modified from the appIsDocument() above, so while I agree that the name isn't the best, I'll leave it as for this PR.

appMode %in% c(
"quarto-static",
"quarto-shiny"
)
}


appHasParameters <- function(appDir, appPrimaryDoc, appMode, contentCategory = NULL) {
# Only Rmd deployments are marked as having parameters. Shiny applications
# may distribute an Rmd alongside app.R, but that does not cause the
Expand Down Expand Up @@ -321,55 +328,3 @@ documentHasPythonChunk <- function(filename) {
matches <- grep("`{python", lines, fixed = TRUE)
return(length(matches) > 0)
}

inferQuartoInfo <- function(appDir, appPrimaryDoc, quarto, metadata) {
if (hasQuartoMetadata(metadata)) {
return(list(
version = metadata[["quarto_version"]],
engines = metadata[["quarto_engines"]]
))
}

if (is.null(quarto)) {
return(NULL)
}

# If we don't yet have Quarto details, run quarto inspect ourselves
inspect <- quartoInspect(
quarto = quarto,
appDir = appDir,
appPrimaryDoc = appPrimaryDoc
)
if (is.null(inspect)) {
return(NULL)
}

list(
version = inspect[["quarto"]][["version"]],
engines = I(inspect[["engines"]])
)
}

hasQuartoMetadata <- function(x) {
!is.null(x$quarto_version)
}

# Run "quarto inspect" on the target and returns its output as a parsed object.
quartoInspect <- function(quarto, appDir = NULL, appPrimaryDoc = NULL) {
# If "quarto inspect appDir" fails, we will try "quarto inspect
# appPrimaryDoc", so that we can support single files as well as projects.
paths <- c(appDir, file.path(appDir, appPrimaryDoc))

for (path in paths) {
args <- c("inspect", path.expand(path))
inspect <- tryCatch(
{
json <- suppressWarnings(system2(quarto, args, stdout = TRUE, stderr = TRUE))
parsed <- jsonlite::fromJSON(json)
return(parsed)
},
error = function(e) NULL
)
}
return(NULL)
}
23 changes: 18 additions & 5 deletions R/deployApp.R
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,13 @@
#' @param forceGeneratePythonEnvironment Optional. If an existing
#' `requirements.txt` file is found, it will be overwritten when this argument
#' is `TRUE`.
#' @param quarto Optional. Full path to a Quarto binary for use deploying Quarto
#' content. The provided Quarto binary will be used to run `quarto inspect`
#' to gather information about the content.
#' @param quarto Should the deployed content be built by quarto?
#' (`TRUE`, `FALSE`, or `NA`). The default, `NA`, will use quarto if
#' there are `.qmd` files in the bundle, or if there is a
#' `_quarto.yml` and `.Rmd` files.
#'
#' (This option is ignored and quarto will always be used if the
#' `metadata` contains `quarto_version` and `quarto_engines` fields.)
Comment on lines +125 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully remember why we required a Quarto path originally. I think that @aronatkins and I discussed not wanting to make assumptions that would break existing workflows, so we kept Quarto support gated behind the parameter? At this point, I definitely prefer this approach.

#' @param appVisibility One of `NULL`, `"private"`, or `"public"`; the
#' visibility of the deployment. When `NULL`, no change to visibility is
#' made. Currently has an effect only on deployments to shinyapps.io.
Expand Down Expand Up @@ -183,7 +187,7 @@ deployApp <- function(appDir = getwd(),
forceUpdate = NULL,
python = NULL,
forceGeneratePythonEnvironment = FALSE,
quarto = NULL,
quarto = NA,
appVisibility = NULL,
image = NULL
) {
Expand Down Expand Up @@ -227,6 +231,15 @@ deployApp <- function(appDir = getwd(),
check_file(recordDir)
}

if (!is_string(quarto)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check negated? Warn on an incoming string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh.

lifecycle::deprecate_warn(
when = "0.9.0",
what = "deployApp(quarto = 'can no longer be a path')",
with = I("`TRUE` instead")
)
quarto <- TRUE
}

# set up logging helpers
logLevel <- match.arg(logLevel)
quiet <- identical(logLevel, "quiet")
Expand Down Expand Up @@ -333,7 +346,7 @@ deployApp <- function(appDir = getwd(),
appDir = appDir,
appFiles = appFiles,
appPrimaryDoc = appPrimaryDoc,
quarto = quarto,
usesQuarto = quarto,
contentCategory = contentCategory,
isCloudServer = isCloudServer,
metadata = metadata
Expand Down
66 changes: 66 additions & 0 deletions R/quarto.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
inferQuartoInfo <- function(metadata, appDir, appPrimaryDoc) {
if (hasQuartoMetadata(metadata)) {
return(list(
version = metadata[["quarto_version"]],
engines = metadata[["quarto_engines"]]
))
}

# If we don't yet have Quarto details, run quarto inspect ourselves
inspect <- quartoInspect(
appDir = appDir,
appPrimaryDoc = appPrimaryDoc
)
if (is.null(inspect)) {
return(NULL)
}

list(
version = inspect[["quarto"]][["version"]],
engines = I(inspect[["engines"]])
)
}

hasQuartoMetadata <- function(x) {
!is.null(x$quarto_version)
}

# Run "quarto inspect" on the target and returns its output as a parsed object.
quartoInspect <- function(appDir = NULL, appPrimaryDoc = NULL) {
# If "quarto inspect appDir" fails, we will try "quarto inspect
# appPrimaryDoc", so that we can support single files as well as projects.
quarto <- quarto_path()
if (is.null(quarto)) {
abort("`quarto` is not installed")
}

paths <- c(appDir, file.path(appDir, appPrimaryDoc))

for (path in paths) {
args <- c("inspect", path.expand(path))
inspect <- tryCatch(
{
json <- suppressWarnings(system2(quarto, args, stdout = TRUE, stderr = TRUE))
parsed <- jsonlite::fromJSON(json)
return(parsed)
},
error = function(e) NULL
)
}
return(NULL)
}

# inlined from quarto::quarto_path()
quarto_path <- function() {
path_env <- Sys.getenv("QUARTO_PATH", unset = NA)
if (is.na(path_env)) {
path <- unname(Sys.which("quarto"))
if (nzchar(path)) {
path
} else {
NULL
}
} else {
path_env
}
}
4 changes: 2 additions & 2 deletions R/writeManifest.R
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ writeManifest <- function(appDir = getwd(),
contentCategory = NULL,
python = NULL,
forceGeneratePythonEnvironment = FALSE,
quarto = NULL,
quarto = NA,
image = NULL,
verbose = FALSE) {
appFiles <- listDeploymentFiles(
Expand All @@ -35,7 +35,7 @@ writeManifest <- function(appDir = getwd(),
appDir = appDir,
appFiles = appFiles,
appPrimaryDoc = appPrimaryDoc,
quarto = quarto,
usesQuarto = quarto,
hadley marked this conversation as resolved.
Show resolved Hide resolved
contentCategory = contentCategory,
)

Expand Down
12 changes: 8 additions & 4 deletions man/deployApp.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions man/writeManifest.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading