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

Shorten the list of dependencies #215

Open
GegznaV opened this issue Jul 15, 2020 · 14 comments
Open

Shorten the list of dependencies #215

GegznaV opened this issue Jul 15, 2020 · 14 comments

Comments

@GegznaV
Copy link
Collaborator

GegznaV commented Jul 15, 2020

The list of dependency packages in DESCRIPTION should be shortened as some fo those packages are not used in hyperSpec anymore.

@cbeleites
Copy link
Owner

IMHO this should include possibly putting vignettes into .Rbuildignoe if that allows us to get rid of dependencies/suggestions.

We'd still have them built in CI (via our separate roxygenize() step), and e.g. included in the pkgdown articles.

But our own CI failing due to a breaking change in the dependency isn't nearly as bad as an ultimatum by CRAN...

@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 17, 2020

I'm not sure if it is a good idea to .Rbuildignore vignettes especially in situations, when a user is offline. It is better to restructure the code in vignettes, disable evaluation, or move some non-essential sections to a separate vignette and ignore only that one.

@bryanhanson Have we removed/commented some sections with additional dependencies until now in the hyperSpec vignette (or elsewhere)? Do you have any ideas what could be done regarding this issue?

@bryanhanson
Copy link
Collaborator

I have only worked on hyperSpec.Rmd and it does not have sections conditionally disabled.

On the main question of this issue, shouldn't the pkgs listed in DESCRIPTION Suggests: or Depends: that are no longer needed in hyperSpec because functions have moved to other packages move to the other pkgs, and then be deleted from hyperSpec? Same idea as deprecated functions and unit tests that have to move. Some of the Suggests: may be exclusively due to their use in vignettes, and some vignettes will move and allow the removal from hyperSpec.

Back to the vignette aspect: one can certainly condition a code chunk based on package availability, but there's no way to know if a given package is available but broken.

@cbeleites
Copy link
Owner

As for the vignettes, I was thinking along the lines that sure sessioninfo::session_info() looks nicer than plain sessionInfo() - but is that actually worth another dependency? (To me, it is not)

@cbeleites
Copy link
Owner

@GegznaV the rgl plots in the laser vignette are conditional on rgl being available.

@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 18, 2020

On sessioninfo::session_info() is not a heavy dependency, R core team is in the list of authors/contributors (so I don't think it will break easily) and it is easy to condition not to use the function if the package is not installed. So I prefer to keep sessioninfo as a suggested dependency.


On rgl. I think, that I disabled all code that uses rgl. It is a heavy dependency and there were plenty of OS-specific issues related to automatic deployment and rgl. So I prefer to get rid of rgl as a dependency.

@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 18, 2020

When I opened this issue, I was mainly thinking of removing R.rsp package.

@cbeleites
Copy link
Owner

R core team is in the list of authors/contributors (so I don't think it will break easily)

that's what I thought with the package XML as well - but beginning of the year it was gone from CRAN sufficiently long to also have hyperSpec being thrown out...


Showcases

Wrt. rgl: I'd still like to show in a vignette/article that if people get it installed, they can use it with hyperSpec. There are other packages that are also needed only as "showcases" (akima) - but IMHO such showcases are one of the main purposes of vignettes/articles.

My thoughts are:

  • I'd like to avoid a hard dependency that could get us thrown off CRAN
  • I'm OK with shipping pre-built vignettes (with R.rsp or otherwise)
  • I'd also be OK with the decision of having some such showcases online only, which would avoid using R.rsp.

We can still have the CI checking that the vignettes can actually be built - so we know when something breaks. But we then don't have the pressure to fix & submit to CRAN within their deadlines.


UX/UI

Have a look at #218 - I implemented a function attach_hySpc() to load most of the hySpc.* package zoo that is installed.

@cbeleites
Copy link
Owner

@bryanhanson deleting dependencies when functionality moves: of course, and (at least for proper dependencies) R CMD check will also tell us if we list dependencies that are never actually used.

I'm thinking a bit higher-level here: is there functionality that we import that is not worth the "exposure"?

@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 18, 2020

I'm OK with shipping pre-built vignettes (with R.rsp or otherwise)

knitr is used to build HTML vignettes now. I do not understand what you have in mind @cbeleites. The only place I find R.rsp mentioned in the package now is the DESCRIPTION file.

that's what I thought with the package XML as well

In which category of dependencies XML was: Depends, Includes, or Suggests? As I can infer from the position of xml2, it was in Depends. It is critically important that packages in Depends and Includes would be reliable and stable. And the story is different for Suggests as these packages are optional.

such showcases are one of the main purposes of vignettes/articles.

Then could there be a separate vignette for those showcases?

@cbeleites
Copy link
Owner

cbeleites commented Jul 19, 2020

Wrt. R.rsp: it provides vignette engine "as.is" which includes prebuilt vignettes, i.e. we build the vignette locally/on CI but R CMD check or the CRAN checks do not build it again. That is, the package will not need to even suggest any particular packages needed to build that vignette. The vignette document (pdf/html) will be available offline for the users, though.

Even though packages in Suggests are optional, CRAN checks include building all vignettes, and if that fails (e.g. because a suggested package changes/becomes unavailable/...) hyperSpec will have an error as well, and we're back in the situation with the deadline.

Depends/Includes vs. Suggests is mostly a difference on user side (you don't have to install suggested packages), but not so much on CRAN/developer side.

@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 20, 2020

Wrt. R.rsp: it provides vignette engine "as.is" which includes prebuilt vignettes, i.e. we build the vignette locally/on CI but R CMD check or the CRAN checks do not build it again. That is, the package will not need to even suggest any particular packages needed to build that vignette. The vignette document (pdf/html) will be available offline for the users, though.

To achieve this, is it enough to have R.rsp in Suggests only and nowhere else in the package mentioned? Understand, that this was useful when PDF vignettes and especially make were used. But I can't understand, how it could be useful for HTML vignettes with knitr as build engine.


About the other dependencies: I think, we'll discuss this on Monday.

@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 20, 2020

1 similar comment
@GegznaV
Copy link
Collaborator Author

GegznaV commented Jul 20, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants