r-lib / pkgbuild

Find tools needed to build R packages
https://pkgbuild.r-lib.org
Other
65 stars 33 forks source link

Allow build args "--no-manual" to affect build in addition to parameter "manual" #120

Closed dgkf closed 2 years ago

dgkf commented 2 years ago

I'm not sure what the intended behavior is here, but it was easy enough to make a PR to illustrate the change.

I updated the build setup steps to consider both args = "--no-manual" as well as manual = TRUE when deciding whether to show message about the lack of pdflatex.

I discovered this issue through the use of rcmdcheck::rcmdcheck, and was surprised to see build messages ("pdflatex not found! Not building PDF manual.") despite passing build_args = "--no-manual". I found that rcmdcheck calls pkgbuild passing manual=TRUE, which then uses this setup function, printing this message even if "--no-manual" is to be used for the build.

I just rearranged the function a bit to first build the build arguments, and use then use the build arguments as the ground-truth before printing messages. This way, either args or manual can affect the inclusion of --no-manual and the message will appropriately reflect the build behavior.


todo:

jimhester commented 2 years ago

Sure, makes sense, do you think it would be worth writing a test case for this as well?

dgkf commented 2 years ago

Sure, can do

dgkf commented 2 years ago

Alright - tests added. Remaining checks look like they're stalled due to an outdated ubuntu image specification for the GHA runners.

Please take a look at how I spoofed the return of has_latex using a global option as a way of locking the latex behaviors for testing. I think this is a good use case for mockery, but I wasn't sure whether this warranted a new Suggests dependency. Let me know if you prefer another mechanism and I can update it if you think there's a better way.

Otherwise, I added a couple tests for the way the latex behavior is fixed, and also explicit tests for mix and matching of various args and manual inputs.

dgkf commented 2 years ago

I noticed today that --no-build-vignettes also follows similar behavior as --no-manual. For example

pkgbuild::build_setup_source(<path>, args = "--no-build-vignettes", vignettes = TRUE, clean_doc = NULL)

Will delete the inst/doc directory in preparation to rebuild vignettes, and proceed to not rebuild/check vignettes. This is the case if the build args are provided through rcmdcheck which hardcodes the default of vignettes = TRUE and with clean_doc being determined by Config/build/clean-inst-doc in DESCRIPTION.

I submitted another small patch in this PR to treat vignettes the same way, first consolidating all the flags into build arguments, and then using the build arguments as the point of truth for behaviors downstream.

jimhester commented 2 years ago

Great, the code and tests look good 👍

Can you please add a bullet to NEWS? It should briefly describe the changes and end with (@yourname, #issuenumber).

After that I would be happy to merge this!

jimhester commented 2 years ago

Thanks again!

dgkf commented 2 years ago

Hey @jimhester - I realized I added my NEWS entry under v1.2.0. I'll add a quick PR to shuffle it over to a new version heading. Sorry to add just a bit more administration to this PR.

jimhester commented 2 years ago

great, thanks for catching that and for opening the new PR, sorry I missed it in my review!