Attempt workaround of CRAN Windows hang on update.dev.pkg.Rd example

[jangorecki]

Try to address unknown hung in https://www.r-project.org/nosvn/R.check/r-release-windows-x86_64/data.table-00check.html

checking tests ...
Check process probably crashed or hung up for 20 minutes ... killed
Most likely this happened in the example checks (?),
if not, ignore the following last lines of example output:
> ### * update.dev.pkg
>
> flush(stderr()); flush(stdout())
>
> ### Name: update.dev.pkg
> ### Title: Perform update of development version of a package
> ### Aliases: update update.dev.pkg
> ### Keywords: data
>
> ### ** Examples
>
> # data.table::update.dev.pkg()
>
>
>
> ### * <FOOTER>
> ###
> cleanEx()
> options(digits = 7L)
> base::cat("Time elapsed: ", proc.time() - base::get("ptime", pos = 'CheckExEnv'),"\n")
Time elapsed: 6.28 0.33 7.64 NA NA
> grDevices::dev.off()
null device
          1
> ###
> ### Local variables: ***
> ### mode: outline-minor ***
> ### outline-regexp: "\\(> \\)?### [*]+" ***
> ### End: ***
> quit('no')

Closes #5304 too by changing the name (see discussion below).

[mattdowle]

That's an interesting idea! This error has been occurring sporadically on Windows CRAN for I'd guess well over a year and I'd emailed Uwe about it asking for any insight. I figured it was timing out due to resource issues on that machine, or perhaps related to tests using 2 threads; i.e. timing out in .Rd examples because those are tested last or something. But since it always times out on this particular example, I suppose it does make sense that it is actually to do with this example! I couldn't see the wood for the trees.
Since it only happens on CRAN's Windows machine and not in our Windows CI it's going to be tricky to test. I'll contact Uwe to see if he's aware of anything to do with empty examples causing hangs on Windows. If that is the problem, it feels like the root cause is something in R and/or CRAN's Windows machine and we shouldn't need to change this example. After all, an empty example should be ok.
Actually, we have --no-manual in .appveyor.yml, so I guess that doesn't test examples. I'll turn on manual to see if it then hangs. [Update: examples are run even when --no-manual so it's ok to keep --no-manual which is there to avoid an error 'pdflatex is not available'.]

[jangorecki]

Yes this one is very strange. I tried to change two things.
Use double ## in case there is some magic that runs single # commented out function calls in examples when "run dontrun" or whatever.
Put any non commented out code to execute in this examples, in case empty examples block cause troubles.

[mattdowle]

(That merge master was to bring in #5398 to make AppVeyor work again.)

I did grep "examples{" *.Rd -A 5 and inspected the output to see if there are any other empty examples. Found these other two.

deprecated.Rd:\examples{
deprecated.Rd-# dummy example section to pass release check that all .Rd files have examples
deprecated.Rd-}

shouldPrint.Rd:\examples{
shouldPrint.Rd-# dummy example section to pass release check that all .Rd files have examples
shouldPrint.Rd-}

update.dev.pkg.Rd:\examples{
update.dev.pkg.Rd-  # data.table::update.dev.pkg()
update.dev.pkg.Rd-}

The only difference that I can see is that the update.dev.pkg.Rd one has two spaces at the start of the line. Using cat -A and od -c confirms they are regular spaces and not special characters. Really strange if those would cause the hang though.

[jangorecki]

This adds a risk of some test jobs running with run-dontrun. IMO it is better to completely protect from running it in examples, either by comment or if(FALSE) ...
I don't see a valid use case for running this example. Yet we should still have it there because the usage section is turned into update method for dev.pkg object, which is clearly wrong: #5304 (comment)

[mattdowle]

Running dontrun seems odd to me but ok. I like that code in dontrun is still parsed and checked which isn't the case for comments. So that leads us to if(FALSE) then. Will do.
Regarding the usage section, our .Rd contains \method{update}{dev.pkg} which tells it it's a method. Can't we drop the \method part there, and remove the \alias{update} at the top too?

[mattdowle]

If it looks like a method of update but isn't actually, then that's the sort of thing that is easier to believe leads to a hang somehow.

[MichaelChirico]

there is _R_CHECK_DONTTEST_EXAMPLES_ but I don't see anything like that for \dontrun

[mattdowle]

Couldn't reproduce the hang using AppVeyor.

Indeed the manual page was misleading :

Now with the S3 method removed, 2 notes about the S3 method overlap, as Jan mentioned in his linked comment above.

* checking S3 generic/method consistency ... NOTE
Found the following apparent S3 methods exported but not registered:
  update.dev.pkg
See section ‘Registering S3 methods’ in the ‘Writing R Extensions’ manual.

* checking Rd \usage sections ... NOTE
S3 methods shown with full name in documentation object 'update.dev.pkg':
  ‘update.dev.pkg’

The \usage entries for S3 methods should use the \method markup and not their full name.
See chapter ‘Writing R documentation files’ in the ‘Writing R Extensions’ manual.

[MichaelChirico]

looks like the best option is to rename it. shouldn't be a "prod" function anyway (maybe used in some docker images? should be searchable), so we can be more aggressive in the deprecation cycle

[mattdowle]

I think we should just get out of the way of stats::update and change the name. We're talking about a utility function here which upgrades the version of data.table, and to the development version too, so it's nothing to do with data.table's API and not like user's code could break. It's worth doing because, although strange, it could somehow be contributing to the hang on CRAN's Windows at this point, plus the manual entry displays as a S3 method which is misleading.
So update_dev_pkg() ?

[mattdowle]

@MichaelChirico :) same conclusion at the same time, hadn't seen yours when I added mine

[MichaelChirico]

hadn't seen yours when I added mine

😄

So update_dev_pkg() ?

yes that's what I had in mind

[mattdowle]

grep -R "update[.]dev[.]pkg"

po/R-zh_CN.po:"Please update: data.table::update.dev.pkg()\n"
po/R-zh_CN.po:"table::update.dev.pkg()\n"
po/R-data.table.pot:msgid "**********\nThis development version of data.table was built more than 4 weeks ago. Please update: data.table::update.dev.pkg()\n**********"
man/data.table.Rd:  update.dev.pkg()
man/update.dev.pkg.Rd:\name{update.dev.pkg}
man/update.dev.pkg.Rd:\alias{update.dev.pkg}
man/update.dev.pkg.Rd:\usage{update.dev.pkg(object="data.table",
man/update.dev.pkg.Rd:  if (FALSE) data.table::update.dev.pkg()
NEWS.md:9. `data.table::update.dev.pkg()` now unloads the `data.table` namespace to alleviate a DLL lock issue on Windows, [#4403](https://github.com/Rdatatable/data.table/issues/4403). Thanks to @drag5 for reporting.
NEWS.md:4. `update.dev.pkg()` gains `type=` to specify if update should be made from binaries, sources or both. [#3148](https://github.com/Rdatatable/data.table/issues/3148). Thanks to Reino Bruner for the detailed suggestions.
NEWS.md:17. `update.dev.pkg` is new function to update package from development repository, it will download package sources only when newer commit is available in repository. `data.table::update.dev.pkg()` defaults updates `data.table`, but any package can be used.
NAMESPACE:export(update.dev.pkg)
README.md:data.table::update.dev.pkg()
R/onAttach.R:      packageStartupMessagef("**********\nThis development version of data.table was built more than 4 weeks ago. Please update: data.table::update.dev.pkg()\n**********")
R/devel.R:update.dev.pkg = function(object="data.table", repo="https://Rdatatable.gitlab.io/data.table", field="Revision", type=getOption("pkgType"), lib=NULL, ...) {
R/devel.R:  # update.dev.pkg fails on windows R 4.0.0, we have to unload package namespace before installing new version #4403

[MichaelChirico]

GitHub search not turning up anything (TBF, it doesn't even match our repo... GH search is... not the best):

https://github.com/search?q=%22update.dev.package%28%22+-org%3ALien182+-org%3Aaomie-x+-org%3ABestduan+-extension%3Atcl+-extension%3Ajs&type=Code

[mattdowle]

I'm thinking it's unlikely Jan would disagree on the name change, so merging now to move on. We can always revisit/revert if needed.

[jangorecki]

Yes, renaming seems to be the best solution.

[jangorecki]

It was a fast fix, fail on cran is gone already and we haven't yet released to cran :)

[mattdowle]

After rename and 1.14.4 is now on CRAN, it's still failing. It seems my original intuition (written above) was right then and the fail on Windows is not related to this .Rd file. It's probably just that this .Rd file is the last one so it's the last thing displayed in the log before the kill happens later in tests.
The log displayed on CRAN does display a last helpful line (below) which was omitted from the original comment above.

checking tests ...
Check process probably crashed or hung up for 20 minutes ... killed
Most likely this happened in the example checks (?),
if not, ignore the following last lines of example output:
>
> ### Name: update_dev_pkg
...
> ### End: ***
> quit('no')
======== End of example output (where/before crash/hang up occured ?) ========

So the example output ends (and update_dev_pkg is the last .Rd file alphabetically) and the last line in the log (with === around it verbatim) implies that the crash/hang happens later. Which is why I thought originally that the update.dev.pkg output was spurious.