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

Make .onLoad() more robust w.r.t. knitr #254

Merged
merged 4 commits into from
Jan 21, 2022
Merged

Make .onLoad() more robust w.r.t. knitr #254

merged 4 commits into from
Jan 21, 2022

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Jan 20, 2022

Closes #253

This generated extensive discussion in Slack, which I'll summarize here.

The root cause is arguably an infelicity in R; specifically in isNamespaceLoaded(). These failures occur when glue is being loaded while knitr's namespace is "sort of" loaded. It's loaded enough for isNamespaceLoaded() to return TRUE, but not loaded enough for knitr::knit_engines$set() to succeed. It feels like this should never happen, yet that's what's causing these failures.

We first really noticed this in stringr's CI for ubuntu-18.04 (devel), e.g. https://github.com/tidyverse/stringr/actions/runs/1725746423. But it has nothing to do with r-devel or CI, but rather it's seen when installing packages from source, in a particular order. The phenomenon has been seen with R 4.1 by a regular user (yihui/knitr#2087). It has been seen in CI with Windows and R 3.6 (https://github.com/tidyverse/stringr/runs/4417596611?check_suite_focus=true). And we've been able to replicate it now.

So why did stringr surface this problem? It seems to be the combination of two recent changes:

  • (This is not new, but is relevant: knitr Imports stringr, stringr Imports glue, glue Suggests knitr.)
    pak::pkg_deps_explain("knitr", "glue")
    #> knitr -> stringr -> glue
  • glue gained this knitr-involving code in .onLoad() in version 1.5.1 on 2021-11-30
    if (isNamespaceLoaded("knitr")) {
      knitr::knit_engines$set(glue = eng_glue, glue_sql = eng_glue_sql, gluesql = eng_glue_sql)
    } else {
      setHook(packageEvent("knitr", "onLoad"), function(...) {
        knitr::knit_engines$set(glue = eng_glue, glue_sql = eng_glue_sql, gluesql = eng_glue_sql)
      })
    }
  • stringr gained importFrom(glue,glue) in its NAMESPACE in tidyverse/stringr@ad7fe11#diff-b6b6854a02ae177f8860f49654ff250c1554d021ae434b6efdbe99d00bdc6055 on 2021-04-18

Nice summary from @gaborcsardi:

... the issue is that the knitr loading triggers stringr loading, which triggers glue loading, and glue thinks that knitr is already loaded, but in fact it is "half-loaded", and not in a usable state yet.

We have consensus that the specific fix in this PR is reasonable and the broader issue re: isNamespaceLoaded() has been raised on the r-devel mailing list (isNamespaceLoaded() while the namespace is loading).

@jennybc
Copy link
Member Author

jennybc commented Jan 21, 2022

Encouraging: tidyverse/stringr#422

@jennybc jennybc changed the title Sort out .onLoad() w.r.t. knitr Make .onLoad() more robust w.r.t. knitr Jan 21, 2022
Also retroactively modify earlier bullet that under-describes a change in 1.5.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error : .onLoad failed in loadNamespace() for 'glue'
1 participant