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

Failures in a file included with @includeRmd will nuke the NAMESPACE #1254

Closed
DavisVaughan opened this issue Oct 1, 2021 · 10 comments · Fixed by #1313
Closed

Failures in a file included with @includeRmd will nuke the NAMESPACE #1254

DavisVaughan opened this issue Oct 1, 2021 · 10 comments · Fixed by #1313
Labels
bug an unexpected problem or unintended behavior namespace 👩‍🚀
Milestone

Comments

@DavisVaughan
Copy link
Member

DavisVaughan commented Oct 1, 2021

If you include a file with @includeRmd that fails during roxygenise(), then your NAMESPACE will be (mostly) nuked, which often puts you in a state where you can't even roxygenise() again to fix it. You end up having to revert the changes made to the NAMESPACE manually, fix the Rmd (or whatever made it fail), and then redocument. (This has bitten the tidymodels team, and me in vctrs quite a few times)

I have a somewhat minimal example here, where this commit changed an Rmd to make it fail, I redocumented, and now the exported function fn() is no longer part of NAMESPACE
DavisVaughan/testfailingroxygen2@ad6f49f

The way this occurs is:

  • roclet_preprocess() has a namespace method that inserts partial information into the NAMESPACE file. It seems to insert information about imports only.
  • roclet_process() will then run, and the Rd method for that is where the Rmd is rendered and subsequently fails.
  • roclet_output() never gets to run, which is where the NAMESPACE is finalized, so we end up with the broken partial NAMESPACE that roclet_preprocess() wrote.

I see two potential solutions:

  • roxy_tag_rd.roxy_tag_includeRmd() could tryCatch() the rmarkdown::render() call? Or maybe pass something to render() to make it not fail.
  • More generally, I'm not entirely sure why roclet_preprocess.roclet_namespace() exists. It seems like the process and output methods for roclet_namespace will basically do the same thing, just with import_only = FALSE. If the preprocess method wasn't run, it seems like we wouldn't get into that problematic state from above because we would never write a partial NAMESPACE to disk. I'm probably missing something though

    roxygen2/R/namespace.R

    Lines 34 to 63 in 6c1e42f

    roclet_preprocess.roclet_namespace <- function(x, blocks, base_path) {
    lines <- blocks_to_ns(blocks, emptyenv(), import_only = TRUE)
    NAMESPACE <- file.path(base_path, "NAMESPACE")
    if (length(lines) == 0 && !made_by_roxygen(NAMESPACE)) {
    return(x)
    }
    results <- c(made_by("#"), lines)
    write_if_different(NAMESPACE, results, check = TRUE)
    invisible(x)
    }
    #' @export
    roclet_process.roclet_namespace <- function(x, blocks, env, base_path) {
    blocks_to_ns(blocks, env)
    }
    #' @export
    roclet_output.roclet_namespace <- function(x, results, base_path, ...) {
    NAMESPACE <- file.path(base_path, "NAMESPACE")
    results <- c(made_by("#"), results)
    # Always check for roxygen2 header before overwriting NAMESPACE (#436),
    # even when running for the first time
    write_if_different(NAMESPACE, results, check = TRUE)
    NAMESPACE
    }
    .
@gaborcsardi
Copy link
Member

This does not happen to me, am I doing this right?

~/works/testfailingroxygen2 main
❯ cat NAMESPACE
# Generated by roxygen2: do not edit by hand

import(rlang)
~/works/testfailingroxygen2 main
❯ R -q -e 'packageVersion("roxygen2")'
> packageVersion("roxygen2")
[1] ‘7.1.2’
>
>
~/works/testfailingroxygen2 main
❯ R -q -e 'devtools::document()'
> devtools::document()
ℹ Updating testfailingroxygen2 documentation
ℹ Loading testfailingroxygen2
Quitting from lines 13-14 (/Users/gaborcsardi/works/testfailingroxygen2/man/rmd/test.Rmd)
Quitting from lines 2-5 (/Users/gaborcsardi/works/testfailingroxygen2/man/rmd/test.Rmd)
Error in eval(expr, envir, enclos) : oh no
Calls: <Anonymous> ... handle -> withCallingHandlers -> withVisible -> eval -> eval
Execution halted
~/works/testfailingroxygen2 main
❯ cat NAMESPACE
# Generated by roxygen2: do not edit by hand

import(rlang)

@DavisVaughan
Copy link
Member Author

If you change the stop("oh no") to something that doesn't error and redocument, you should get a export(foo) put back in the NAMESPACE. Then you should be back in a "correct" state.

So then you can add stop("oh no") back in and see that when you redocument, it should remove that export(foo) again, which is the problem.


I committed all the changes that roxygen2 made, which was probably confusing

@gaborcsardi
Copy link
Member

Got it. Two workarounds, that are IMO better than @includeRmd anyway:

  1. If you only need the code chunk once, then include it directly in the R file:

    #' ```{r, error = FALSE}
    #' stop("oh no")
    #' ```
    
  2. If you need it in multiple files, then use child documents:

    #' ```{r, child = "man/rmd/test.Rmd", error = FALSE}
    #' ```
    

@DavisVaughan
Copy link
Member Author

Just want to clarify that the errors are unexpected, i.e. I would never set error = FALSE before running the entire Rmd. I don't expect any errors and would like to be notified if any errors occur. I just don't want it to nuke the NAMESPACE file in the process

@hadley
Copy link
Member

hadley commented Nov 15, 2021

Maybe the NAMESPACE that just records the imports could also include the standard export pattern thing, so if the second step fails at least you're not completely hosed?

Or maybe we should have some way to roll back to the previous namespace if the whole process fails?

And maybe we also need something in load_all() to handle the problem of a broken NAMESPACE? That also comes up when you have a merge conflict in NAMESPACE.

@hadley hadley added bug an unexpected problem or unintended behavior namespace 👩‍🚀 labels Mar 29, 2022
@hadley hadley added this to the v7.1.3 milestone Mar 29, 2022
@hadley
Copy link
Member

hadley commented Mar 29, 2022

Possible duplicate of #1144

@hadley
Copy link
Member

hadley commented Mar 31, 2022

@DavisVaughan do you happen to have an example where after the fixing the problem you can't redocument? In https://github.com/DavisVaughan/testfailingroxygen2, if I comment out the error, I can still rebuild the package.

roxygen2 currently does two passes to solve the a boostrapping problem. Imagine if we didn't have the pre-processing step and wrote this code:

#' @importFrom foo someS3generic
#' @export
someS3generic.character <- function(x) 3

The first time we run it, we can't tell that someS3generic.character is an S3 method, so export(someS3generic.character) is added to the NAMESPACE. But the second time we run it, someS3generic will be imported, and we'll write exportS3method(someS3generic,character).

Or imagine that someS3generic has now been deleted from foo, and we need to remove the @importFrom. If we don't preprocess the NAMESPACE before load_all() then we'll get an error when trying to load the package code, and would have to manually repair the NAMESPACE to continue.

But maybe the pre-processing step could somehow preserve all the existing lines that aren't imports?

@hadley
Copy link
Member

hadley commented Mar 31, 2022

This approach seems promising:

parsed <- parse("NAMESPACE", keep.source = TRUE)
is_import <- function(x) is_call(x, c("import", "importFrom", "importClassesFrom", "importMethodsFrom"))
unlist(lapply(attr(parsed, "srcref")[!vapply(parsed, is_import, logical(1))], as.character))

@DavisVaughan
Copy link
Member Author

DavisVaughan commented Mar 31, 2022

Yea, I think the super broken state is related to S3 methods.

I have created an updated "broken" branch in that repo. You can check it out or do usethis::pr_fetch(1) to get it.

  1. Open man/rmd/test.Rmd
  2. Run CMD+SHIFT+D to document, note that everything is working
  3. Uncomment the stop()
  4. Document again, it should fail - It nukes NAMESPACE
  5. Comment the stop() out
  6. Document again, it should still fail - it complains about not being able to find an S3 method for "double"
  7. Manually revert the NAMESPACE
  8. You should now be able to document

hadley added a commit that referenced this issue Mar 31, 2022
Now preserves all non-import directives.

Fixes #1254
@hadley
Copy link
Member

hadley commented Mar 31, 2022

Ah of course, because you're using an exported function that's no longer exported.

hadley added a commit that referenced this issue Apr 1, 2022
Now preserves all non-import directives.

Fixes #1254
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 namespace 👩‍🚀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants