r-lib / pak

A fresh approach to package installation
https://pak.r-lib.org
646 stars 57 forks source link

Inclusion of indirect `"LinkingTo"` dependencies in lockfile #485

Closed nbenn closed 1 year ago

nbenn commented 1 year ago

Consider the following setup: Root package {tidyverse} Imports {readxl} LinkingTo {cpp11}.

In a lockfile generated as

lockfile_create(
  "tidyverse",
  dependencies = list(
    direct = "Imports",
    indirect = "LinkingTo"
  )
)

Am I wrong to expect {cpp11} in there, irrespectively of whether my OS/repos setting supports binary installs?

gaborcsardi commented 1 year ago

Yeah, if it is not needed for you, then it won't be in the lockfile.

nbenn commented 1 year ago

Thanks for getting back to me. Is there a way to disable this, i.e. to force inclusion of such dependencies?

It seems to me that currently there is no consensus on whether or not such dependencies are actually optional or not. For example, when using pak in a CI setup that goes on to deploy to connect, absence of such dependencies seems to be a problem. Deployment fails with

...
* Content depends on package "cpp11" but it is not installed. Please
  resolve before continuing.
...

Happy to provide more details or try to come up with a reproducible example for the issue I'm describing if you feel this is worth looking into.

gaborcsardi commented 1 year ago

If you want to use the lockfile on another platform, then create it for that platform. E.g. for linux set the platform to source before creating the lockfile:

options(pkg.platforms = "source")
nbenn commented 1 year ago

Thanks for that. I wasn't aware. It isn't the perfect solution however.

Please feel free to close this issue again if what I'm hoping for is beyond the scope of what pak is supposed to to.

Both the CI system and the system running connect are capable of/set up for binary installs. I was therefore hoping to use this more efficient mechanism over source installs.

The problem I think (not entirely sure here) is that packrat on connect isn't as smart as pak and considers indirect "LinkingTo" dependencies as "hard" dependencies even though there actually not (given the specific Ubuntu/PPM binary setup over traditional Linux/CRAN).

The ideal solution in my mind would therefore be to

Again: the situation I'm in is probably a bit of an edge-case and I understand if you don't think this is a problem that should be addressed by pak. I just wanted to bring this to your attention I case you feel otherwise.

gaborcsardi commented 1 year ago

Can you tell me more how you use packrat to install the packages in the pak lockfile?

nbenn commented 1 year ago

I don't use packrat to install anything.

I use pak to install packages on my CI system which then runs checks and if successful, initiates deployment to Connect. For that I need a manifest.json, which per docs can be created using rsconnect::writeManifest(). This internally creates a packrat "snapshot".

The deployment however fails with messages like

* Content depends on package "cpp11" but it is not installed. Please
  resolve before continuing.

I haven't pinpointed where exactly they are coming from, but what I believe is happening is that packrat considers indirect "LinkingTo" dependencies as "hard" and refuses to go on because pak does not (in this specific set up) and they consequently were not installed.

One way to fix this would be to have pak install indirect "LinkingTo" dependencies even if they are not strictly required, ideally using binary releases (if possible) instead of sources.

Does that make sense? I'm happy to look into this further if you're interested. But as said earlier, I'd find it perfectly understandable if you tended towards something along the lines of "this is more the problem of packrat/Connect/etc. than that of pak".

gaborcsardi commented 1 year ago

That makes sense. It is unlikely that this would be fixed in packrat I think, but we can try to support it in pak.

gaborcsardi commented 1 year ago

How about having an option that would make pak also include LinkingTo, even for binaries?

nbenn commented 1 year ago

Yeah, sounds great to me! Thanks.

gaborcsardi commented 1 year ago

Closed by https://github.com/r-lib/pkgdepends/pull/316. Will be in tomorrow's nightly pak build.