-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
-Z bindeps
currently does not fully support dependencies on packages in a registry.
The core issue is that the feature resolver needs to know which target a dependency is for (the target="..."
field).
The feature resolver currently uses the dependency data from the dependency resolver, which gets its information from the index. However, the index does not contain the necessary information.
Theoretically the feature resolver could lazily download packages, but we would like to avoid that.
One point of uncertainty I have is whether or not the index should have just the target
field, or if it should have all the new fields (target
, artifact
, lib
). I think it only needs to know about target
. However, I lean towards keeping all of the fields just for the sake of being complete in case we want the information in the future.
The following is an outline of what I think it will take to resolve this:
Publishing
- Add the fields to
NewCrateDependency
. This is the structure that is used when publishing.- We may also want to add the
v
field. (See below for discussion about updating thev
field to 3.) A concern is that we don't want to force a registry to do a bunch of processing to figure out the correct value of thev
field, so cargo could theoretically provide that information to simplify registry implementations.
- We may also want to add the
- Beware that there is already a
target
field which has a different meaning. The new target field will need a different name. - Please also bump the version of the crates-io package since this will be a semver-breaking change.
- These fields should only be populated when
-Z bindeps
is used. - Publishing without
-Z bindeps
should be an error.
Reading the index
- Add the fields to
RegistryDependency
. This is the structure that is used when deserializing the index. - The new fields should be guarded by a bump in the
v
field to version3
. This helps ensure that older versions of Cargo won't see these new entries when resolving since they won't know how to handle them correctly (and things may change before it is stabilized).- This check will need to depend on whether
-Zbindeps
is used. If-Zbindeps
is used, then it should be> 3
, otherwise it should be> INDEX_V_MAX
which is 2. That ensures that cargo will only read the entries with the-Z
flag passed.
- This check will need to depend on whether
Pre-download
- Cargo tries to download only the minimum of packages it needs. This is done in
download_accessible
. However, it needs to download before the feature resolver runs. This is primarily so that the feature resolver can know which packages are proc-macros (which is not stored in the index). Thetarget
field throws a wrench into this problem, because that can require downloading for extra targets. But we don't want to makedownload_accessible
too complicated. It is intentionally primitive, otherwise it would need to be as complicated as the feature resolver itself. I don't know how to solve this problem. If it can be extended with only a small amount of code, that's great. Otherwise, I'd be reluctant to make it too complicated.
If we can't figure out a simple solution, then an alternative is to do a best effort in download_accessible
, and then in the feature resolver download packages on an as-needed basis. I think this can essentially be done by changing this expect
to a ?
so that any download errors get reported. I think that should work, but it needs some thought and analysis.
Handle RustcTargetData
- The feature resolver needs to query the
RustcTargetData
structure for information about targets (which are collected fromrustc
). Unfortunately the point whereRustcTargetData
is created doesn't know about whichtarget
fields are needed for dependencies (it is done far too early). Some possible different solutions:- Make
RustcTargetData
support the ability to lazily add new targets. However that requires access to aWorkspace
, which I think may make things quite messy. - In
resolve_ws_with_opts
, take a mutable reference toRustcTargetData
. After the dependency resolver runs, walk the dependency graph and collect every target that should be added and updateRustcTargetData
. - Change
resolve_ws_with_opts
so that instead of taking a reference toRustcTargetData
, make it responsible for creatingRustcTargetData
. After running the dependency resolver, it can walk the graph and populate it with all the targets. It can then stuff the result intoWorkspaceResolve
. (I have no idea if this is workable.)
- Make
Update feature resolver?
I actually don't think there is anything to do here, just writing this to note that it should be examined to ensure it works.
Testing
- Add/update any tests related to all of the above.
- Update
cargo-test-support/src/registry.rs
to accommodate all of the above. It needs to generate artifact entries in the index in the same way as the above new code does. It also needs to set thev
key to 3 when using artifact deps.
Update crates.io
This should be done later, closer to when we are gearing up to stabilize bindeps. Adding the fields to the index is a permanent decision, so we need to be sure it has the right design.
- Add fields to
EncodableCrateDependency
, this is the structure it receives from cargo. - Add fields to
Dependency
, this is the structure it uses for serializing to the index. - The
v
field here needs to be set to 3 if it contains a dependency with an artifact dependency. (See commentary above about possibly passing thev
field in via the JSON API so that a registry does not need to process it.) - Consider adding some input validation. For example, it may want to put limits on the lengths of strings. I don't think it needs to otherwise be very restrictive, but crates.io people may have further ideas.
- Add the appropriate tests. I think it would go somewhere in
src/tests/krate/publish.rs
. - Consider whether or not the artifact information should be displayed on the website. If the crates.io team would like to track this information, then it would also need to be stored in the database (which means also doing a schema update). The model is defined around here. I am no expert in diesel, so I can't provide much more guidance here. Talk with the crates.io team to see if this is something desired.
cc @Byron