Skip to content

Conversation

@motorailgun
Copy link
Contributor

@motorailgun motorailgun commented Nov 5, 2025

Succeeding #15003.

Piror to this, using a GitHub pull request URLs as dependencies would just fail because of HTTP errors as it's simply not a git repository. This PR implements some help messages on such cases for users to know why it's failing, and how to fix it.

Close #15001.

@rustbot rustbot added the A-manifest Area: Cargo.toml issues label Nov 5, 2025
@motorailgun motorailgun force-pushed the git-pr-url branch 3 times, most recently from 3a91b48 to 87f00a3 Compare November 5, 2025 16:21
@epage
Copy link
Contributor

epage commented Nov 5, 2025

As a heads up, we have some helpful suggestions for PRs at our contrib guide, including

  • Using atomic commits, having a clear history that communicates your change, breaking down a change into refactors and the actual fix to make review clear
  • Where it works (which error message changes it usually applies), add the tests in a commit before the fix with the tests passing, showing the old behavior and then update the tests in your fix commit so they show the new behavior and the diff between the commits shows how the behavior changed.

@epage
Copy link
Contributor

epage commented Nov 5, 2025

Ah, the title says this is for an error but the code looks to be related to a warning

@motorailgun motorailgun changed the title fix: add error message for github PR url in dep fix: add warning for github PR url in dep Nov 6, 2025
@motorailgun motorailgun changed the title fix: add warning for github PR url in dep fix: add warning for github PR URL in dependency Nov 6, 2025
@motorailgun motorailgun force-pushed the git-pr-url branch 2 times, most recently from 69d5d4f to d771595 Compare November 6, 2025 09:13
github-merge-queue bot pushed a commit that referenced this pull request Nov 6, 2025
### What does this PR try to resolve?

Explicitly call out that we use rustc's diagnostic style guide, e.g.
#16207 (comment)

### How to test and review this PR?
@rustbot rustbot added the A-git Area: anything dealing with git label Nov 9, 2025
@motorailgun motorailgun force-pushed the git-pr-url branch 2 times, most recently from 55fb6f2 to f2836a7 Compare November 9, 2025 12:22
@motorailgun
Copy link
Contributor Author

motorailgun commented Nov 9, 2025

Just have pushed changes reflecting kind reviews. The commit history is kind of a mess, so I'll clean it out once the change is confirmed appropriate. I'd also like to work on this but I think that should be another PR.

@motorailgun motorailgun marked this pull request as ready for review November 9, 2025 12:34
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to clean up commit history :)

View changes since this review

" `git = \"{}\" rev = \"{}\"` \n",
" to specify pull requests as dependencies' revision."
),
url, repo_url, rev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer inline arg like git = "{repo_url}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but format! does not capture variables when the format string is expanded from a macro (in this case, concat!). I'd prefer readability of concat! over iteration of push_string(format!(...)) s.

@rustbot
Copy link
Collaborator

rustbot commented Nov 10, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@motorailgun motorailgun changed the title fix: add warning for github PR URL in dependency feat: emit help messages for github pull request url in dependency Nov 10, 2025
@motorailgun
Copy link
Contributor Author

Cleared out commits, rebased on latest master. Also edited PR comment to reflect current implementation.

@motorailgun
Copy link
Contributor Author

Wasn't aware of these, thanks!

@motorailgun motorailgun requested a review from epage November 10, 2025 18:49
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage epage enabled auto-merge November 10, 2025 21:40
@epage epage added this pull request to the merge queue Nov 10, 2025
Merged via the queue into rust-lang:master with commit 4b0e263 Nov 10, 2025
25 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2025
bors added a commit to rust-lang/rust that referenced this pull request Nov 12, 2025
Update cargo

10 commits in 445fe4a68f469bf936b2fd81de2c503b233a7f4f..2d4fa139552ebdd5f091a1401ed03f7dc62cb43f
2025-11-07 18:08:19 +0000 to 2025-11-12 15:56:06 +0000
- feat: Add unstable rustc-unicode flag (rust-lang/cargo#16243)
- fix(package): all tar entries timestamp be the same (rust-lang/cargo#16242)
- feat: emit help messages for github pull request url in dependency (rust-lang/cargo#16207)
- docs: fix comments for alternative registry fns (rust-lang/cargo#16235)
- add into_value utility function for inheritableField (rust-lang/cargo#16234)
- fix(command-vendor): strip_prefix panic in cp_sources method (rust-lang/cargo#16214)
- fix(lock): Be moore direct in the error message (rust-lang/cargo#16233)
- fix(lock): In error, differentiate between creating and updating lockfile (rust-lang/cargo#16227)
- fix(cli): Refer to commands, not subcommands (rust-lang/cargo#16226)
- fix(run): Help teach about argument escaping (rust-lang/cargo#16225)
@rustbot rustbot added this to the 1.93.0 milestone Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-git Area: anything dealing with git A-manifest Area: Cargo.toml issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow specifying pull request deps using the github PR link

4 participants