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

writeManifest() assumes a Quarto deployment in the presence of a .qmd #658

Closed
gadenbuie opened this issue Feb 3, 2023 · 8 comments · Fixed by #828
Closed

writeManifest() assumes a Quarto deployment in the presence of a .qmd #658

gadenbuie opened this issue Feb 3, 2023 · 8 comments · Fixed by #828
Labels
bug an unexpected problem or unintended behavior dependencies 🐀
Milestone

Comments

@gadenbuie
Copy link
Member

gadenbuie commented Feb 3, 2023

If we intend to deploy an .Rmd with an adjacent, but unrelated, .qmd (something we'd make use of in the Shiny R Markdown document, for example), writeManifest() will assume 1) Quarto is required and 2) that the deployment is a quarto-shiny deployment.

Here's a small reprex, writing app.Rmd and supplement.qmd into a temp folder.

tmpdir <- withr::local_tempdir()
withr::local_dir(tmpdir)

writeLines("app.Rmd", text = c(
  "---",
  "title: test rmd",
  "output: html_document",
  "runtime: shinyrmd",
  "---",
  "",
  "```{r context='server'}",
  "",
  "```"
))

writeLines("supplement.qmd", text = c(
  "just a qmd file"
))

If we tell writeManifest() that the primary document is app.Rmd, it will still expect that we provide a quarto binary path.

rsconnect::writeManifest(quarto = NULL, appPrimaryDoc = "app.Rmd")
#> Error in inferAppMode(appDir = appDir, appPrimaryDoc = appPrimaryDoc, : Attempting to deploy Quarto content without Quarto metadata. Please provide the path to a quarto binary to the 'quarto' argument.

If we provide the quarto path, the resulting manifest will choose app.Rmd as the primary document but will infer a quarto-shiny app mode.

rsconnect::writeManifest(
  quarto = Sys.which("quarto"),
  appPrimaryDoc = "app.Rmd"
)
jsonlite::read_json("manifest.json")[c("metadata", "quarto")]
#> $metadata
#> $metadata$appmode
#> [1] "quarto-shiny"
#> 
#> $metadata$primary_rmd
#> [1] "app.Rmd"
#> 
#> $metadata$primary_html
#> NULL
#> 
#> $metadata$content_category
#> NULL
#> 
#> $metadata$has_parameters
#> [1] FALSE
#> 
#> 
#> $quarto
#> $quarto$version
#> [1] "1.2.313"
#> 
#> $quarto$engines
#> $quarto$engines[[1]]
#> [1] "knitr"

If we delete the adjacent .qmd file we get the expected behavior (but without being able to make (easy) use of the .qmd in our deployed app).

unlink("supplement.qmd")

rsconnect::writeManifest(quarto = NULL, appPrimaryDoc = "app.Rmd")
jsonlite::read_json("manifest.json")[c("metadata", "quarto")]
#> $metadata
#> $metadata$appmode
#> [1] "rmd-shiny"
#> 
#> $metadata$primary_rmd
#> [1] "app.Rmd"
#> 
#> $metadata$primary_html
#> NULL
#> 
#> $metadata$content_category
#> NULL
#> 
#> $metadata$has_parameters
#> [1] FALSE
#> 
#> 
#> $<NA>
#> NULL
Session info
sessioninfo::session_info()
#> ─ Session info ───────────────────────────────────────────────────────────────
#>  setting  value
#>  version  R version 3.6.3 (2020-02-29)
#>  os       macOS  10.16
#>  system   x86_64, darwin15.6.0
#>  ui       X11
#>  language (EN)
#>  collate  en_US.UTF-8
#>  ctype    en_US.UTF-8
#>  tz       America/New_York
#>  date     2023-02-03
#>  pandoc   2.19.2 @ /usr/local/bin/ (via rmarkdown)
#> 
#> ─ Packages ───────────────────────────────────────────────────────────────────
#>  package     * version date (UTC) lib source
#>  cli           3.6.0   2023-01-09 [1] CRAN (R 3.6.3)
#>  digest        0.6.31  2022-12-11 [1] CRAN (R 3.6.3)
#>  evaluate      0.20    2023-01-17 [1] CRAN (R 3.6.3)
#>  fansi         1.0.4   2023-01-22 [1] CRAN (R 3.6.3)
#>  fastmap       1.1.0   2021-01-25 [1] CRAN (R 3.6.2)
#>  fs            1.6.0   2023-01-23 [1] CRAN (R 3.6.3)
#>  glue          1.6.2   2022-02-24 [1] CRAN (R 3.6.3)
#>  htmltools     0.5.4   2022-12-07 [1] CRAN (R 3.6.3)
#>  jsonlite      1.8.4   2022-12-06 [1] CRAN (R 3.6.3)
#>  knitr         1.42    2023-01-25 [1] CRAN (R 3.6.3)
#>  lifecycle     1.0.3   2022-10-07 [1] CRAN (R 3.6.3)
#>  magrittr      2.0.3   2022-03-30 [1] CRAN (R 3.6.3)
#>  packrat       0.9.0   2023-01-09 [1] CRAN (R 3.6.3)
#>  pillar        1.8.1   2022-08-19 [1] CRAN (R 3.6.3)
#>  pkgconfig     2.0.3   2019-09-22 [1] CRAN (R 3.6.0)
#>  purrr         1.0.1   2023-01-10 [1] CRAN (R 3.6.3)
#>  R.cache       0.16.0  2022-07-21 [1] CRAN (R 3.6.3)
#>  R.methodsS3   1.8.2   2022-06-13 [1] CRAN (R 3.6.3)
#>  R.oo          1.25.0  2022-06-12 [1] CRAN (R 3.6.3)
#>  R.utils       2.12.0  2022-06-28 [1] CRAN (R 3.6.3)
#>  reprex        2.0.1   2021-08-05 [1] CRAN (R 3.6.2)
#>  rlang         1.0.6   2022-09-24 [1] CRAN (R 3.6.3)
#>  rmarkdown     2.20    2023-01-19 [1] CRAN (R 3.6.3)
#>  rsconnect     0.8.29  2023-01-09 [1] CRAN (R 3.6.3)
#>  sessioninfo   1.2.2   2021-12-06 [1] CRAN (R 3.6.2)
#>  styler        1.7.0   2022-03-13 [1] CRAN (R 3.6.3)
#>  tibble        3.1.8   2022-07-22 [1] CRAN (R 3.6.3)
#>  utf8          1.2.2   2021-07-24 [1] CRAN (R 3.6.2)
#>  vctrs         0.5.2   2023-01-23 [1] CRAN (R 3.6.3)
#>  withr         2.5.0   2022-03-03 [1] CRAN (R 3.6.3)
#>  xfun          0.36    2022-12-21 [1] CRAN (R 3.6.3)
#>  yaml          2.3.7   2023-01-23 [1] CRAN (R 3.6.3)
#> 
#>  [1] /Users/garrick/Library/R/3.6/library
#>  [2] /Library/Frameworks/R.framework/Versions/3.6/Resources/library
#> 
#> ──────────────────────────────────────────────────────────────────────────────
@gadenbuie
Copy link
Member Author

An additional detail, if you provide a path to quarto, the inferred app mode is again quarto-shiny even if the primary document is an .Rmd and no nearby .qmd exists.

# after removing supplemental.qmd

rsconnect::writeManifest(quarto = Sys.which("quarto"), appPrimaryDoc = "app.Rmd")
jsonlite::read_json("manifest.json")[c("metadata", "quarto")]
#> $metadata
#> $metadata$appmode
#> [1] "quarto-shiny"
#> 
#> $metadata$primary_rmd
#> [1] "app.Rmd"
#> 
#> $metadata$primary_html
#> NULL
#> 
#> $metadata$content_category
#> NULL
#> 
#> $metadata$has_parameters
#> [1] FALSE
#> 
#> 
#> $quarto
#> $quarto$version
#> [1] "1.2.313"
#> 
#> $quarto$engines
#> $quarto$engines[[1]]
#> [1] "knitr"

@hadley
Copy link
Member

hadley commented Feb 17, 2023

The challenge is that we currently have a bit of a circular logic — do we determine the primaryDoc using the appMode, or do we determine the appMode from the appPrimaryDoc. But given that the only control the user has is the appPrimaryDoc, we should make that determine in the appMode.

@aronatkins
Copy link
Contributor

The example from @gadenbuie exposes another complication: Quarto can support content.Rmd, even though files with that extension are typically processed by rmarkdown. If you give additional hints (a path to a Quarto executable, Quarto details in metadata, a *.qmd file, etc.), that is a strong signal to use Quarto.

If you say "deploy content.Rmd" without any additional signal, we'll see that as R Markdown content.

The IDE often knows more about the runtime requirements for the content (providing the metadata information), but other contexts need to give additional signal.

@hadley
Copy link
Member

hadley commented Feb 17, 2023

@aronatkins I don't quite get how this interface is supposed to work. It feels weird to me to say if you want to use quarto, set quarto to the path to your quarto binary, when determining the quarto binary is something that's way easier for us to do.

More discussion on the current interface in #595.

@hadley
Copy link
Member

hadley commented Feb 17, 2023

One possible way forward would be to change the meaning of the quarto. Currently NULL, the default, means don't use quarto, and a string (giving the path to the quarto binary) means to use quarto. What if we added three new (preferred) options:

  • TRUE: use quarto, not Rmd
  • FALSE: use Rmd, not quarto
  • NA (the new default): use quarto if there are .qmds or a _quarto.yml

@gadenbuie
Copy link
Member Author

gadenbuie commented Feb 17, 2023

@hadley's proposal would resolve a lot of the issues I ran into. It'd be much cleaner for rsconnect to find the path and for the user to have more control over whether or not quarto should be used. If the quarto path needs to be modified, a user could set QUARTO_PATH which could be picked up by rsconnect with something like Sys.getenv("QUARTO_PATH", Sys.which("quarto")), right?

@hadley hadley added bug an unexpected problem or unintended behavior dependencies 🐀 labels Feb 21, 2023
@aronatkins
Copy link
Contributor

I'm OK trying the proposal, refinement around the details:

  • metadata contains Quarto information:
    Use Quarto using the incoming metadata and to not run quarto inspect. This case is driven by the IDE publishing workflows and quarto-r.

    When we are given Quarto metadata, the value of the quarto is inconsequential, as we do not need to run quarto inspect. We could err if there is a conflict (err if !is.null(metadata[["quarto_version"]]) && quarto==FALSE).

  • metadata does NOT contain Quarto information:

    • TRUE: use Quarto, not Rmd, using whatever Quarto we discover using an approach like quarto_path from our helper-paths.R (which was inspired by quarto::quarto_path).
    • <path>: use Quarto, not Rmd, using the specified Quarto.
    • FALSE: use Rmd, not Quarto.
    • NA (the new default): Use Quarto (using a quarto_path approach) if there are .qmd or a _quarto.yml.

@jthomasmock
Copy link

This resolves my issue (that Hadley closed). Agreed with the TRUE FALSE NA options 👍

I don't know how many customers will have scripts with a hard coded quarto path or quarto_path() but I think we should be clear if this is preferred (still accepts raw path) or breaking change (accepts only logical or NA).

@hadley hadley added this to the 1.0.0 milestone Apr 27, 2023
hadley added a commit that referenced this issue May 1, 2023
I'm assuming that the previous interface of supplying a path is mostly an accident of history (not a desired UI) and hasn't been used to select between different versions of quarto.

Fixes #658
hadley added a commit that referenced this issue May 2, 2023
Replace path specification (needed for historical reasons) with `TRUE`/`FALSE`/`NA`.

Fixes #658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior dependencies 🐀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants