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

Custom config_file renamed during render #1307

Closed
debruine opened this issue Feb 7, 2022 · 9 comments
Closed

Custom config_file renamed during render #1307

debruine opened this issue Feb 7, 2022 · 9 comments
Assignees

Comments

@debruine
Copy link

debruine commented Feb 7, 2022

I have two different config files for rendering different versions of a book (including or excluding chapters). They have two different names, _bookdown_v1.yml and _bookdown_draft.yml, so I have to set the config_file argument when I render:

bookdown::render_book(config_file = "_bookdown_v1.yml")

During the rendering process, the config file is renamed to _bookdown.yml. This can cause several problems:

  • If that file is open in RStudio, I get a warning that the file has disappeared; rendering pauses until I deal with the modal window
  • If I have another file called _bookdown.yml, it is overwritten, so you can't keep a default file if you ever want to use a custom config file
  • If R crashes or I force-quit during render, _bookdown.yml is not renamed back to _bookdown_v1.yml

There might be a good reason that the file has to be renamed, but I think if you copied it to _bookdown.yml rather than renaming it, it would avoid the first and third problems.

As always, thank you so much for all you do for the R community!

session_info

R version 4.1.0 (2021-05-18)
Platform: x86_64-apple-darwin17.0 (64-bit)
Running under: macOS Mojave 10.14.6, RStudio 1.4.1106

Locale: en_GB.UTF-8 / en_GB.UTF-8 / en_GB.UTF-8 / C / en_GB.UTF-8 / en_GB.UTF-8

Package version:
base64enc_0.1.3 bookdown_0.24 digest_0.6.29 evaluate_0.14 fastmap_1.1.0 glue_1.6.0
graphics_4.1.0 grDevices_4.1.0 highr_0.9 htmltools_0.5.2 jquerylib_0.1.4 jsonlite_1.7.3
knitr_1.37 magrittr_2.0.1 methods_4.1.0 rlang_0.4.12 rmarkdown_2.11 stats_4.1.0
stringi_1.7.6 stringr_1.4.0 tinytex_0.36 tools_4.1.0 utils_4.1.0 xfun_0.29
yaml_2.2.1

@cderv
Copy link
Collaborator

cderv commented Feb 7, 2022

Hi,

If that can help understand the current behavior, here are some context:

Currently, config file needs to be name _bookdown.yml and located at the root of the project. So when another config file is provided in config_file what happens is the following.

bookdown/R/render.R

Lines 98 to 106 in 6ae8900

if (config_file != '_bookdown.yml') {
unlink(tmp_config <- tempfile('_bookdown_', '.', '.yml'))
if (file.exists('_bookdown.yml')) file.rename('_bookdown.yml', tmp_config)
file.rename(config_file, '_bookdown.yml')
on.exit({
file.rename('_bookdown.yml', config_file)
if (file.exists(tmp_config)) file.rename(tmp_config, '_bookdown.yml')
}, add = TRUE)
}

  • If there is already a _bookdown.yml then, it is saved in a temp file (by moving the file)
  • The config_file content provided is then rename to _bookdown.yml
  • At the end, on.exit close will revert back by moving the file in there initial place.

So it seems indeed that we are currently file.rename()-ing the YAML file and not just copying. I guess we could just apply the logic and copy 🤔 @yihui is there is reason to rename config_file to _bookdown.yml instead of just copying ?

If I have another file called _bookdown.yml, it is overwritten, so you can't keep a default file if you ever want to use a custom config file

The content is saved in a tempfile and restored at the end of the rendering - so you can have a default file and some custom files.

If R crashes or I force-quit during render, _bookdown.yml is not renamed back to _bookdown_v1.yml

We are dependant of on.exit() behavior here I guess. Even if we copy _bookdown_v1.yml, so we do not remove it, we need to change _bookdown.yml content if it exists. R crashing would then prevent restoring the initial content if on.exit is not called.
We could probably not save in a temp file but in a _bookdown.yml.bak or _bookdown.yml.old maybe ? But writing in project directory is less safe than in a temp file when we need to name the file.

Another solution would be to support directly the user provided filename as a config file, without any copying and replacing.

Maybe something like this

diff --git a/R/render.R b/R/render.R
index ac87d259..dacd2793 100644
--- a/R/render.R
+++ b/R/render.R
@@ -95,17 +95,8 @@ render_book = function(
     "versions of bookdown."
   )

-  if (config_file != '_bookdown.yml') {
-    unlink(tmp_config <- tempfile('_bookdown_', '.', '.yml'))
-    if (file.exists('_bookdown.yml')) file.rename('_bookdown.yml', tmp_config)
-    file.rename(config_file, '_bookdown.yml')
-    on.exit({
-      file.rename('_bookdown.yml', config_file)
-      if (file.exists(tmp_config)) file.rename(tmp_config, '_bookdown.yml')
-    }, add = TRUE)
-  }
-
   on.exit(opts$restore(), add = TRUE)
+  opts$set(config_file = config_file)
   config = load_config()  # configurations in _bookdown.yml
   output_dir = output_dirname(output_dir, config)
   on.exit(xfun::del_empty_dir(output_dir), add = TRUE)
diff --git a/R/utils.R b/R/utils.R
index 8f6a67e1..8f47e0f4 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -69,10 +69,11 @@ get_base_format = function(format, options = list()) {
   do.call(format, options)
 }

-load_config = function() {
-  if (length(opts$get('config')) == 0 && file.exists('_bookdown.yml')) {
+load_config = function(config_file = '_bookdown.yml') {
+  config_file = opts$get('config_file') %n% config_file
+  if (length(opts$get('config')) == 0 && file.exists(config_file)) {
     # store the book config
-    opts$set(config = rmarkdown:::yaml_load_file('_bookdown.yml'))
+    opts$set(config = rmarkdown:::yaml_load_file(config_file))
   }
   opts$get('config')
 }

I wonder if that would be enough - load_config() is used in several places to retrieve default config. We need to be sure it gets the correct file. 🤔

@yihui you know better - I let you comment before doing any PR.

@yihui
Copy link
Member

yihui commented Feb 24, 2022

@yihui is there is reason to rename config_file to _bookdown.yml instead of just copying ?

Now I think we should copy instead of renaming.

Another solution would be to support directly the user provided filename as a config file, without any copying and replacing.

That sounds like a better idea.

@steeleb
Copy link

steeleb commented Oct 26, 2023

Hi all, I'll note I have a related issue:

I have a folder in my root directory called 'config_files' - it conatins .yml files are unrelated to my bookdown and belong to a different aspect of my repository/project. When I render my book (the Rmd's and associated yml files are in a folder called 'bookdown', which I've set up to export the output to a folder in the root directory called 'docs'), it moves the entirety of the 'config_files' folder, along with all files within the folder to a folder called _bookdown_files.

Is there any way to prevent this behavior from happening when I render a bookdown?

@cderv
Copy link
Collaborator

cderv commented Oct 26, 2023

@steeleb I believe this happens only because folder ends with _files. If you rename like config-files it does not happen right ?

We list files in *_files and *_cache folder

bookdown/R/utils.R

Lines 448 to 454 in e717c5e

files_cache_dirs = function(dir = '.') {
if (!dir_exists(dir)) return(character())
out = list.files(dir, '_(files|cache)$', full.names = TRUE)
out = out[dir_exists(out)]
out = out[basename(out) != '_bookdown_files']
out
}

In R Markdown ecosystem, *_files folder are common has this is where a Rmd document would write its resources. I believe a config.Rmd would write to config_files folder too... which would conflict with your case.

@yihui should we make that a rule and warn about this in a bookdown project ? Or find a way to exclude some directories ?
Like if it was named _config_files with a _ first ?

@yihui
Copy link
Member

yihui commented Oct 27, 2023

@steeleb Your issue is a different one, not related to the original issue here, but thanks for the report anyway! Next time you can file a new issue.

@cderv In theory, those _files and _cache folders must have their corresponding Rmd files. I'll add a filter.

@yihui yihui closed this as completed in a0ef873 Oct 27, 2023
yihui added a commit that referenced this issue Oct 27, 2023
this should fix the problem reported at #1307 (comment), in which case config_files was moved by mistake
@yihui
Copy link
Member

yihui commented Oct 27, 2023

@cderv I just applied the patch you provided above to fix @debruine's issue.

@steeleb Your issue should also be fixed now.

You can install the development version via

remotes::install_github('rstudio/bookdown')

@yihui yihui self-assigned this Oct 27, 2023
@steeleb
Copy link

steeleb commented Oct 27, 2023

Oh how interesting, @cderv - thanks for elucidating. I tried a number of different names, but they all ended in ..._files so I was not able to resolve on my end. Thank you @yihui for coding up a solution here. This is great. Sorry for riding the coattails of @debruine's issue!

@cderv
Copy link
Collaborator

cderv commented Oct 27, 2023

Thanks @yihui!

Copy link

github-actions bot commented May 1, 2024

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants