Skip to content

Add build_manual and build_vignettes to install functions#377

Merged
jimhester merged 2 commits intomasterfrom
feat/build_vignettes
Jun 7, 2019
Merged

Add build_manual and build_vignettes to install functions#377
jimhester merged 2 commits intomasterfrom
feat/build_vignettes

Conversation

@jimhester
Copy link
Member

Fixes #353

"System command error"
safe_build_package(test_path("invalidpkg"), build_opts = opts, build_manual = FALSE, build_vignettes = FALSE, out, quiet = TRUE, use_pkgbuild = TRUE),
"System command error",
class = "system_command_status_error"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just silences a warning from the newer testthat, as this is a classed error message.

@jimhester
Copy link
Member Author

An alternative implementation would be to have the install_*() functions call normalize_build_opts() themselves and just pass the normalized build_opts onwards. I thought it was a bit nicer to keep that as an implementation detail in install(), though it required more work to pass the arguments correctly.

@jimhester jimhester requested a review from jennybc June 6, 2019 15:27
@jimhester
Copy link
Member Author

I think I would prefer to do this in remotes rather than putting wrappers in devtools, as we also discussed.

@jimhester jimhester force-pushed the feat/build_vignettes branch from 3fcce81 to 493a24b Compare June 6, 2019 15:42
Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

install-github.R Outdated
safe_build_package <- function(pkgdir, build_opts, dest_path, quiet, use_pkgbuild = !is_standalone() && pkg_installed("pkgbuild")) {
normalize_build_opts <- function(build_opts, build_manual, build_vignettes) {
if (!isTRUE(build_manual)) {
build_opts <- c(build_opts, "--no-manual")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for parallelism with setdiff(), should this c() and the one for build_vignettes be union() and then you could ditch the unique()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good suggestion, yeah

@jennybc
Copy link
Member

jennybc commented Jun 6, 2019

Sad news: I fetched this PR and, while there are no obvious signs of trouble with install_github("r-lib/fs", build_vignettes = TRUE), I don't seem to have vignettes when it's done. fs is a good test case.

@jimhester
Copy link
Member Author

Hmm, it seems to work fine for me, this is what I see, the important bit to look for is

─ installing the package to build vignettes
✔ creating vignettes (25.1s)

> install_github("r-lib/fs", build_vignettes = TRUE)
Downloading GitHub repo r-lib/fs@master
   checking for file ‘/private/var/folders/dt/r5s12t392tb5sk181j3gs4zw0000gn
✔  checking for file ‘/private/var/folders/dt/r5s12t392tb5sk181j3gs4zw0000gn
/T/Rtmp8ZUbVN/remotes168402bb6da04/r-lib-fs-1555cd0/DESCRIPTION’
─  preparing ‘fs’:
✔  checking DESCRIPTION meta-information ...
─  cleaning src
─  installing the package to build vignettes
✔  creating vignettes (25.1s)
─  cleaning src
─  checking for LF line-endings in source and make files and shell scripts
─  checking for empty or unneeded directories
─  building ‘fs_1.3.1.9000.tar.gz’

Installing package into ‘/Users/jhester/.temp’
(as ‘lib’ is unspecified)
* installing *source* package ‘fs’ ...
** libs
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c dir.cc -o dir.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c error.cc -o error.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c file.cc -o file.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c fs.cc -o fs.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c id.cc -o id.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c link.cc -o link.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c path.cc -o path.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c utils.cc -o utils.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c unix/getmode.cc -o unix/
getmode.o
clang++  -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./
libuv/include -I. -pthread -I"/Users/jhester/Library/R/3.5/library/Rcpp/incl
ude" -I/usr/local/include   -fPIC  -Wall -g -O2  -c RcppExports.cpp -o RcppE
xports.o
cp -p -f libuv/m4/lt_obsolete.m4 libuv/m4/lt~obsolete.m4
(cd libuv \
                && touch aclocal.m4 \
                && touch -r aclocal.m4 configure Makefile.in \
        && CC="clang" CFLAGS="-Wall -g -O2  -fPIC " AR="ar" RANLIB="ranlib"
LDFLAGS="-L/usr/local/lib" ./configure --quiet)
rm: cannot remove 'conftest.dSYM': Is a directory
rm: cannot remove 'conftest.dSYM': Is a directory
rm: cannot remove 'conftest.dSYM': Is a directory
rm: cannot remove 'conftest.dSYM': Is a directory
make --directory=libuv \
                HAVE_DTRACE=0
  CC       src/libuv_la-fs-poll.lo
  CC       src/libuv_la-inet.lo
  CC       src/libuv_la-threadpool.lo
  CC       src/libuv_la-version.lo
  CC       src/libuv_la-uv-common.lo
  CC       src/unix/libuv_la-async.lo
  CC       src/unix/libuv_la-core.lo
  CC       src/unix/libuv_la-dl.lo
  CC       src/unix/libuv_la-fs.lo
  CC       src/unix/libuv_la-getaddrinfo.lo
  CC       src/unix/libuv_la-getnameinfo.lo
  CC       src/unix/libuv_la-loop-watcher.lo
  CC       src/unix/libuv_la-loop.lo
  CC       src/unix/libuv_la-pipe.lo
  CC       src/unix/libuv_la-poll.lo
  CC       src/unix/libuv_la-process.lo
  CC       src/unix/libuv_la-signal.lo
  CC       src/unix/libuv_la-stream.lo
  CC       src/unix/libuv_la-tcp.lo
  CC       src/unix/libuv_la-thread.lo
  CC       src/unix/libuv_la-timer.lo
  CC       src/unix/libuv_la-tty.lo
  CC       src/unix/libuv_la-udp.lo
  CC       src/unix/libuv_la-bsd-ifaddrs.lo
  CC       src/unix/libuv_la-darwin.lo
  CC       src/unix/libuv_la-darwin-proctitle.lo
  CC       src/unix/libuv_la-fsevents.lo
  CC       src/unix/libuv_la-kqueue.lo
  CC       src/unix/libuv_la-proctitle.lo
  CCLD     libuv.la
clang++ -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_look
up -single_module -multiply_defined suppress -L/Library/Frameworks/R.framewo
rk/Resources/lib -L/usr/local/lib -o fs.so dir.o error.o file.o fs.o id.o li
nk.o path.o utils.o unix/getmode.o RcppExports.o ./libuv/.libs/libuv.a -pthr
ead -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,Cor
eFoundation
installing to /Users/jhester/.temp/fs/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (fs)
Adding ‘fs_1.3.1.9000.tgz’ to the cache
> browseVignettes(package = "fs")

fs might be a tricky case because it is a devtools dependency via usethis, as not everything might get unloaded cleanly...

@jimhester jimhester force-pushed the feat/build_vignettes branch from 795417b to b9f3097 Compare June 6, 2019 16:55
@jimhester jimhester force-pushed the feat/build_vignettes branch from b9f3097 to 5d96704 Compare June 6, 2019 16:55
@jennybc
Copy link
Member

jennybc commented Jun 6, 2019

OK I just had a successful fs experiment. I sure thought I had installed remotes from this PR, but I must have only checked out the branch. Sorry for the hiccup.

@jennybc
Copy link
Member

jennybc commented Jun 6, 2019

Sad news: I fetched this PR and, while there are no obvious signs of trouble with install_github("r-lib/fs", build_vignettes = TRUE), I don't seem to have vignettes when it's done. fs is a good test case.

The silent failure of my first attempt is presumably yet another example of ... quietly absorbing build_vignettes = TRUE and then doing nothing with it.

* Fix bug in internal `parse_deps()` where test of valid comparison operators
failed due to trailing whitespaces in DESCRIPTION fields (@LiNk-NY, #366)

* `install_*()` functions gain `build_manual` and `build_vignette` arguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `install_*()` functions gain `build_manual` and `build_vignette` arguments
* `install_*()` functions gain `build_manual` and `build_vignettes` arguments

@jimhester jimhester force-pushed the feat/build_vignettes branch from 5d96704 to 928d236 Compare June 7, 2019 12:33
@jimhester jimhester merged commit 9d0c647 into master Jun 7, 2019
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.

Argument for building vignettes in install_*()

2 participants