r-lib / remotes

Install R packages from GitHub, GitLab, Bitbucket, git, svn repositories, URLs
https://remotes.r-lib.org/
Other
332 stars 152 forks source link

reverted changes discussion #790

Open maksymiuks opened 6 months ago

maksymiuks commented 6 months ago

Hi, I saw that some past contributions of mine (#783, #733) were reverted. The commit message mentions some issues with devtools, and I was hoping you could expand on this so that we can find an implementation that incorporates this feature while integrating nicely with devtools.

For added background, this capability was contributed to support an enterprise use case which requires more precise control over the way that remote dependencies are prioritized. This is primarily to support situations where internal remote dependencies are used for development, but in production, we want to prioritize only published production packages from a CRAN-like internal repository.

We are open to contributing to devtools to address any breaking changes and make these changes possible, or iterating on the design of the feature if some aspect of it has proved to be problematic.

In the future, I would appreciate communication when contributions are being reverted, as our business process has already begun to anticipate the upcoming feature.

gaborcsardi commented 6 months ago

Sorry about that. Unfortunately devtools re-exports these functions and also includes a manual page for them, with the old signature. So they break R CMD check in devtools.

I re-added the changes now to dev remotes, so that might solve the issue for you for now, but you'll need to keep using dev remotes.

The devtools approach is not great, because that effectively means that we cannot change the signature of these functions. So either we'd need to change devtools first to remove the manual pages for the re-exported functions, or to dynamically generated docs in devtools.

dgkf-roche commented 6 months ago

If the type signature is constrained, would it be easier to incorporate this change if it could be configured using a global option?

Personally I try to avoid global state like this, but it might offer enough control while being a viable path within the bounds that devtools has set.

hadley commented 4 months ago

@dgkf-roche is there any chance you could switch to using pak instead?

dgkf-roche commented 4 months ago

@dgkf-roche is there any chance you could switch to using pak instead?

@hadly Yeah, we're definitely looking in that direction. Up until recently we were limited by a few known bugs for private git and GitLab remotes, but those have both been patched in the dev version so we're just waiting for those to release.

Beyond that known issue, this was a good nudge for us to look more closely at pak to see if it would work for us after the upcoming changes. We found two key behaviors that we'd want to be able to control before it would be a viable path. Both are in the spirit of providing a more controlled ecosystem for enterprise use. I'll file these separately as feature requests to pak, but in short

  1. The ability to be explicit about the prioritization (or omission) of possible dependency sources, be it from options(repos), Additional_repositories and Remotes.
  2. Avoiding the implicit inclusion of CRAN/BioC
gaborcsardi commented 4 months ago

Prioritization would be nice, but that's also not (really) implemented in remotes, which just does the same as install.packages(), and takes the latest version of every package. Which is the same as what pak does if you specify upgrade = TRUE.

Additional_repositories is not something that we can support properly, because it is not included in the repository metadata. I.e. if have a local package we see it in DESCRIPTION, if a package is installed from a CRAN-like repo then we don't know if it has such a field or not. We can support it for local, url, git, etc. package sources, just like we support Remotes.

You can avoid Bioc with a flag: options(pkg.use_bioconductor = FALSE). To avoid CRAN name a (dummy?) repo CRAN. I'll a flag for ignoring CRAN as well.

dgkf-roche commented 4 months ago

Prioritization would be nice, but that's also not (really) implemented in remotes, which just does the same as install.packages()

For installation, prioritizing specific repos this is something we can manage using options(available_package_filters) - that's not a problem. It poses an issue when remotes or pak is resolving dependencies which may come from either a remote or a repository. In some situations we want to be absolutely sure that any package used comes from specified repositories, even if the package is declared in Remotes.

You can avoid Bioc with a flag: options(pkg.use_bioconductor = FALSE). To avoid CRAN name a (dummy?) repo CRAN. I'll a flag for ignoring CRAN as well.

Ah, this is great to know. Then that should be sufficient for our needs, but does feel a bit like we're tricking pak which always feels a bit uncomfortable from a maintenance perspective.

Additional_repositories is not something that we can support properly, because it is not included in the repository metadata.

That's okay, and even preferred! Our goal is to ensure that it isn't used. In most (all?) cases we want to ignore this field. On rare occasion we'll check a package from a remote source. In these situations, we'd like to be more explicit about when and how any additional repositories might be used.