Allow specifying pull request deps using the github PR link

[joshka]

Problem

From time to time I need to submit a PR to a lib and then want to rely on that PR in my local code.
Every time I do this I have to search the docs to work out the specific syntax that I need.
It would be nice if I could just use GitHub's PR syntax for this (or at the very least the error message should point me in the right direction)

Proposed Solution

Option 1. Support this as-is:

[dependencies]
foo = { version = "1", git = "https://github.com/bar/foo/pull/123" }

I understand why this might not be ideal even if it's the most developer friendly fix for this. So alternatively:

Option 2. Provide a better error message for this scenario that provides the right syntax:

Current error message:

    Updating git repository `https://github.com/bar/foo/pull/123`
warning: spurious network error (3 tries remaining): unexpected http status code: 404; class=Http (34)
warning: spurious network error (2 tries remaining): unexpected http status code: 404; class=Http (34)
warning: spurious network error (1 tries remaining): unexpected http status code: 404; class=Http (34)
error: failed to get `foo` as a dependency of package `some-package v0.1.0 (/Users/joshka/local/some-package)`

Caused by:
  failed to load source for dependency `some-package`

Caused by:
  Unable to update https://github.com/bar/foo/pull/123

Caused by:
  failed to fetch into: /Users/joshka/.cargo/git/db/161-9e1cf949600a4ac3

Caused by:
  network failure seems to have happened
  if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  unexpected http status code: 404; class=Http (34)

Expected error message (something like):

The git dependency for `foo` points at the pull request url. Fix this by changing to `foo = { version = "1" git = "https://github.com/bar/foo" rev = "refs/pull/123/head"

Notes

I understand that there's other code forges than GitHub, but it's reasonable to fix the 99% case even when other cases are not fixed.

[epage]

For Option 1, we'd be pulling github into our compatibility story which is not great.

Option 2 is more viable. One question would be what hosting services (and enterprise deployments) should we support.

As patches are likely what should be encouraged in these scenarios, we should make sure we support those as well with whatever we do.

Speaking of patches, another option is to have support added in https://crates.io/crates/cargo-override in some form. I don't remember if the author would like to eventually have it merged into cargo. As its likely not there yet, experimenting with this now is a lot more feasible. Even when its merged, it might be possible to "just work". This would likely be a UX command like cargo add where there are few compatibility guarantees. The question would be on whether this would qualify as not having a compatibility guarantee or not.

[joshka]

This really comes down to how much to apply Postel's law.

For Option 1, we'd be pulling github into our compatibility story which is not great.

The reason why I don't think this is a big problem is that we're talking about a string. It's not like this would impose any requirement on any consumer to load a library to interpret this / be compatible. The fact that we're discussing this issue on GitHub is proof enough suggests that avoiding being compatible with GitHub shouldn't be a high priority.

The one thing where I'm likely downplaying in that is that once you support a specific value like this, then you support it forever (or at least until you bump a version field). This seems intuitively not to be a huge problem, but I'd seek your opinion there as I may be missing some context.

As patches are likely what should be encouraged in these scenarios, we should make sure we support those as well with whatever we do.

Not quite sure I'm grokking this. Are you talking about also supporting direct links to diff patch urls on github, or are you talking about future updates to the code which would be written for this? If it's the former, then this is just another branch of the logic, so fairly simple. If it's the latter then the answer is that any naive implementation of either option is not a one-way door to implementing further support.

Speaking of patches, another option is to have support added in https://crates.io/crates/cargo-override in some form. I don't remember if the author would like to eventually have it merged into cargo. As its likely not there yet, experimenting with this now is a lot more feasible. Even when its merged, it might be possible to "just work". This would likely be a UX command like cargo add where there are few compatibility guarantees. The question would be on whether this would qualify as not having a compatibility guarantee or not.

I think that a lot of people see the Cargo.toml file as the primary user interface that drives dependency resolution. Pick any random crate README and you'll see "add this to your Cargo.toml" more often than "cargo add foo". To me this says that cargo should handle errors nicely.

[weihanglo]

Thanks for the feedback! Here are problems of option one I am aware of

• According to Postel's law (TIL), it is actually more conservative because Cargo will need to map GitHub pull request URLs to a specific revision i.e., refs/pull/987/head. This is actually less liberal in what cargo accepts from users. If GitHub has changed anything, some version of Cargo will start failing because of the hardcode mappings.
• The git field currently point to the repository URL, regardless it is SSH or HTTPS URLs. If we had changed its meaning, there might be a mild backward compatibility hazard. That doesn't mean we cannot extend funcionality for existing fields, but still something need to consider.
• Cargo internally uses the git field to uniquely identify the source of each package. While an extra normalization for GitHub isn't too hard to implement, there are some other places like cargo install, cargo add, and cargo pkgid also supports Git URLs. For example, should wpackage id specification supports it like registry+https://github.com/rust-lang/cargo/pull/5#0.52.0? When no, then it looks like a half-done feature and adds more inconsistency. If yes, it really adds more complexity and a huge stable commitment.
• Also, not only Cargo relies on the git field, some other community-supported external tools read and parse the field and source informations. Manifest format changes are usually (actually I see it always) tedious news for external tool maintainers. It would be a communication-intensive process for both Cargo maintainers and tools maintainers. And when we're in the middle of it, some user experience might degrade.

Sorry I was just dumping my thoughts here. Not meant to devalue the discussion.

[joshka]

According to Postel's law (TIL), it is actually more conservative because Cargo will need to map GitHub pull request URLs to a specific revision i.e., refs/pull/987/head. This is actually less liberal in what cargo accepts from users. If GitHub has changed anything, some version of Cargo will start failing because of the hardcode mappings.

If GitHub changes that, then any manifests which use refs already will fail. (That said, there's surprisingly few places that take advantage of this. I would have thought it would be much more. https://github.com/search?q=path%3ACargo.toml+%22rev+%3D+%5C%22refs%22&type=code)

Cargo internally uses the git field to uniquely identify the source of each package. While an extra normalization for GitHub isn't too hard to implement, there are some other places like cargo install, cargo add, and cargo pkgid also supports Git URLs. For example, should wpackage id specification supports it like registry+#5 (comment)? When no, then it looks like a half-done feature and adds more inconsistency. If yes, it really adds more complexity and a huge stable commitment.

I think this is a sufficient argument to not do Option 1.

Also, not only Cargo relies on the git field,

Also a good argument for not doing Option 1

Sorry I was just dumping my thoughts here. Not meant to devalue the discussion.

No need to apologize. You convinced me (easily) that my assumptions were wrong by giving me the missing info I needed to adjust them. Thanks!

I have a PR incoming for Option 2. See #15003

[epage]

As patches are likely what should be encouraged in these scenarios, we should make sure we support those as well with whatever we do.

Not quite sure I'm grokking this. Are you talking about also supporting direct links to diff patch urls on github, or are you talking about future updates to the code which would be written for this? If it's the former, then this is just another branch of the logic, so fairly simple. If it's the latter then the answer is that any naive implementation of either option is not a one-way door to implementing further support.

If you are temporarily overriding a dependency to use the result of a PR, then the expected workflow for that is to patch the dependency, rather than edit the dependency specification. The command cargo override helps with this.

This means any fix for this workflow should apply to patches as equally or more than dependency specifications.

[joshka]

If you are temporarily overriding a dependency to use the result of a PR, then the expected workflow for that is to patch the dependency, rather than edit the dependency specification. The command cargo override helps with this.

This means any fix for this workflow should apply to patches as equally or more than dependency specifications.

Got it. Of the 3 interpretations of patches I saw the other 2 and missed the obvious one. I mentally mapped what you're referring to as a (patch) override, rather than a patch.