Behaviour of load_code in roxygenize()

[jennybc]

This devtools discussion around updating devtools::document() in light of roxygen v6.1.0 needs to continue here:

r-lib/devtools#1849

Excerpt to remind what this is about:

... the helpers = FALSE behaviour (in the sense of load_all(pkg$path, helpers = FALSE)) has been lost. Is that worth taking a stand on?

Two additional points raised by @jimhester in the other thread:

Maybe we should be calling pkgload::load_all(helpers = FALSE) in roxygen2 and maybe export_all = FALSE to more closely mimic a normal package namespace? I am not sure what roxygen2 assumes in regards to exports.

Also potentially recompile = FALSE, which would be a way to fix #771, but maybe would cause other issues if the dll was not built.

[spennihana]

#776 also complains about this. :)

As Dirk suggested, I've now locally pinned to an older version of roxygen that doesn't do the compile steps.

6.1.0 adds a lot of overhead: does it clean all non-.Rd artifacts (do I need to do this before creating a tar for CRAN)? is this package doing other surprising things that I now have to think through? why does this package do more than read .R/.cpp files and produce the man directory and NAMESPACE files? Should I just write my own tool and eschew roxygen2 altogether?

I do not mean to disparage any devs here, and I likely think this huge change may have been an accident (since I cannot find any documentation or helpful description of this change in the NEWS).

It would be great to get some proper clarification.

[gaborcsardi]

The thing is, most R packages don't even need to be loaded properly for generating the documentation. E.g. if the package only contains function objects, and there is no tricky stuff in .onLoad, e.g. C or R code that creates or modifies objects in the package namespace, then no compilation or even proper loading is needed.

But changing the loading code to just collate + parse is probably too late now, it would break some packages. So I agree with @jimhester that an option to force a recompile is probably best. Ideally it should be a package option, that you can set in the Roxygen field of DESCRIPTION. I don't think people would want to turn it on for all packages.

[hadley]

@spennihana this change was made because the previous attempt failed for people and I received multiple complaints that devtools::document() and roxygen2::roxygenise() produced different results. It is described in the news as "roxygenise() uses pkgload::load_all() instead of a home grown solution to simulate package loading"

@gaborcsardi the decision to do run-time documentation was made a long-time time ago, and I'm still confident that, overall, it was the right decision. (Also technically all packages modify the package namespace because they use <- to create bindings to functions 😛)

[gaborcsardi]

@hadley I agree that it is the correct thing to do. But it is also overkill for most packages. E.g. in igraph I have nasty workarounds to avoid recompilation, because R/roxygen always recompiles, and compilation takes several minutes. OK, this is extreme, but constant recompilation can still be annoying.

Anyway, how about having a package-level option for this in DESCRIPTION/Roxygen, but making TRUE the default. And if people don't want to recompile, they'll just set it to FALSE?

[hadley]

As @jimhester suggested in another thread, we could just set recompile = FALSE when we call load_all()

[spennihana]

@hadley, thanks for the further note.

I did see the NEWS update regarding load_all, but my question is whether roxygen2 is even required since devtools::document offers identical behavior. What is the future of roxygen2/document and should I migrate away from one or the other? Is there any choice that I can make that will give me package-building stability, or should I expect continued churn?

[hadley]

@spennihana you can expect roxygen2 to continue to change as I respond to user feedback. This change had consequences that I did not expect, so we'll try to dial it back in the next release.

[hadley]

Unfortunately there's no way to completely suppress compilation in pkgload::load_all() — recompile = FALSE is already the default, but what we actually need is compile = FALSE. I'll implement that in pkgload and then once that's on CRAN (hopefully later this week), I'll do pkgload::load_all(compile = FALSE, helpers = FALSE), which should resolve the worst of the problems related to this change.

[hadley]

Should be better now. Can someone else confirm that this fixes the problem before I push to CRAN?

[hadley]

I need to release today to fix issues with CRAN R cmd check and I'll hope silence indicates no major problems.