-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Allow path + registry dependencies #4844
Conversation
src/cargo/ops/cargo_install.rs
Outdated
@@ -384,7 +384,8 @@ fn select_pkg<'a, T>(mut source: T, | |||
None => None, | |||
}; | |||
let vers = vers.as_ref().map(|s| &**s); | |||
let dep = Dependency::parse_no_deprecated(name, vers, source.source_id())?; | |||
let dep = Dependency::parse_no_deprecated(name, vers, source.source_id(), | |||
source.source_id())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and a couple of other places I'm just setting the registry ID to the source ID since it's easier and we won't end up in the publish codepath from here. It's a bit gross though. Maybe it should be optional?
The test failure seems like a flake? |
Hm could this be possible without specifying an extra field to |
The other place to put it would be |
Hm sorry so actually thinking again about this. Is it safe to assume that when you publish a crate then all your path dependencies will be published to the same registry? I think that means we could infer this, right? |
I think it'd be pretty weird to have a "mixed" workspace, but I don't know if I'd want to lock that decision in. It'd be an easy minimal step though. |
If we went that direction I think we would want to enforce that behavior. Not sure how straightforward that would be though. |
Would we actually be locking ourselves in? I'd see it as the more conservative route and then if we liked we could allow simultaneous path/registry keys in the future |
a3c5b44
to
0ec7f68
Compare
Updated to add a setter for registry_id rather than modifying the constructor. |
@bors: r+ |
📌 Commit 0ec7f68 has been approved by |
Allow path + registry dependencies Closes #4843 Do we have any infrastructure for testing what metadata a publish actually sends to the registry? r? @withoutboats
☀️ Test successful - status-appveyor, status-travis |
Closes #4843
Do we have any infrastructure for testing what metadata a publish actually sends to the registry?
r? @withoutboats