Config/Needs/*#459
Conversation
…override the source for remote Needs packages
| packages | ||
| } | ||
|
|
||
| /// Enrich needs entries: if a package appears in Config/Needs/* without a version |
There was a problem hiding this comment.
Not sure how necessary this is. I dug in and pak/setup-r-dependencies does allow version requirements on packages within Config/Needs/*, but when testing @mduncans and I found https://github.com/r-lib/bit64/blob/main/DESCRIPTION.
Since all package users won't be using rv, Matt is thinking all of these dependencies will still need to be listed under the Suggests, so this add was as a developer UX benefit of not having to list all requirements twice when following that pattern
| .map(|(key, deps)| { | ||
| let entries = deps | ||
| .iter() | ||
| .map(|d| NeedsEntry::Package(d.clone())) |
There was a problem hiding this comment.
it can't return a remote? why is needs parsing not returning exactly the right shape?
There was a problem hiding this comment.
We're locking them as Vec<Dependency> to stay consistent with how other packages are locked, therefore having to map them to the proper structure.
It can't be a remote, similar to pkgs that are listed as Remotes in a DESCRIPTION file. Those packages are just listed as "normal" dependencies and the package is locked as being from the remote source
| /// Fetches Config/Needs/* entries for `resolved_dep` if requested by `item`, then filters to | ||
| /// only the requested keys. Returns `Err` if the DESCRIPTION cannot be fetched/parsed, or if | ||
| /// any requested need key is absent from the DESCRIPTION. | ||
| fn enrich_and_filter_needs( |
There was a problem hiding this comment.
can that be handled when resolving the deps? This feels a bit weird to do it while we're queueing packages
| /// *comes from*, not a requirement to fetch it from git. If the package name is listed in | ||
| /// `prefer_repositories_for`, repos are tried first; git is used as a fallback (or primary | ||
| /// if not preferred). | ||
| fn resolve_needs_remote<E: CommandExecutor + Clone + 'static>( |
There was a problem hiding this comment.
the resolver currently handles the various types of remote and this feels tackled on and is not a source. Can we not add the needs packages with their req to the queue and let it handle everything?
| #[serde(default)] | ||
| needs: Vec<String>, | ||
| #[serde(default)] | ||
| install_all_needs: bool, |
There was a problem hiding this comment.
is the end result when it's true different from install_suggestions = true?
There was a problem hiding this comment.
Yes. There is no assumption/guarantee that Install/Needs/* packages are a subset of Suggests. In fact, the most common pattern of Config/Needs/website explicitly lists packages that would otherwise not be installed
| /// Config/Needs/* entries, keyed by need key (e.g. "website"). | ||
| /// Only populated when the user specified `needs = [...]` or `install_all_needs = true`. | ||
| #[serde(default, deserialize_with = "deserialize_needs")] | ||
| pub needs: HashMap<String, Vec<Dependency>>, |
There was a problem hiding this comment.
hahsmap means the order of the lockfile is no longer deterministic. How about a Vec of tuples instead
| let arr: Array = deps.iter().map(|d| d.as_toml_value()).collect(); | ||
| needs_table.insert(key, Value::Array(arr)); | ||
| } | ||
| table.insert("needs", Item::Value(Value::InlineTable(needs_table))); |
There was a problem hiding this comment.
ah it looks like InlineTable uses IndexMap internally so it's still deterministic.A bit too risky IMO to rely on the undocumented private api of a crate. Ideally we would only have Vec
|
Meta:
|
| } else { | ||
| item.needs | ||
| .iter() | ||
| .filter_map(|k| resolved_dep.needs.get(k)) |
There was a problem hiding this comment.
that swallows a potential invalid need, should it be an error?
2 participants
In R, there are 3 core levels to declare dependencies: Imports, Depends, and Suggests. Suggests is where all non-required packages are placed, whether they be for development (testthat, covr, etc.) or optional functionality. On the other hand, languages like Rust allow a user to create "Features" that allow for the expansion of the core library to allow users to do other things, at the expense of additional dependencies.
Currently in
rv, if a user would like to use the optional functionality, they have to either declareinstall_suggestions = trueor explicitly list the dependencies which they'd like to use for the feature.install_suggestionswould install the development packages which may not be useful to the user's project and listing the dependencies is not a good user experience and can cause confusion downstream when review dependencies why a package is being used.Others before have adopted the
Config/Needs/*pattern (so much so it is documented in the R package book).setup-r-dependenciesuses these fields to give users ways to specify other dependencies used for their CI task, with the main use beingConfig/Needs/website.This PR allows a user to now specify
needsto gain access to dependencies listed inConfig/Needs/*. This meansrvis able to play with the existing ecosystem of packages withConfig/Needs/*forsetup-r-dependenciesand allow package developers to continue expanding the configuration abilities within an R package.