Skip to content
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

If rev is added to git url cargo gives no warnings and has unexpected behavior #8241

Closed
mjarkk opened this issue May 14, 2020 · 9 comments · Fixed by #8297 or rust-lang/rust#72901
Closed
Labels
A-git Area: anything dealing with git C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@mjarkk
Copy link
Contributor

mjarkk commented May 14, 2020

Problem
I have 2 git repos in my Cargo.toml:

[dependencies]
juniper-actix = { git = "https://github.com/graphql-rust/juniper#91a33539", package = "juniper_actix" }
juniper = { git = "https://github.com/graphql-rust/juniper#91a33539" }

When cargo downloads / compiles these packages it shows the #91a33539 2 times:

> cargo build
    Updating git repository `https://github.com/graphql-rust/juniper#91a33539`
   Compiling juniper_codegen v0.14.2 (https://github.com/graphql-rust/juniper#91a33539#91a33539)
   Compiling juniper v0.14.2 (https://github.com/graphql-rust/juniper#91a33539#91a33539)
   Compiling juniper_actix v0.1.0 (https://github.com/graphql-rust/juniper#91a33539#91a33539)

Steps

  1. Run cargo init
  2. Add the deps shown above to the dependencies
  3. Run cargo build

Possible Solution(s)
Show an error with a note to add rev = ".." to the dep or support adding a rev to the URL

Notes

Output of rustup show:

> rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/mark/.rustup

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu
nightly-x86_64-unknown-linux-gnu (default)

active toolchain
----------------

nightly-x86_64-unknown-linux-gnu (directory override for '/home/mark')
rustc 1.45.0-nightly (7ced01a73 2020-04-30)
@mjarkk mjarkk added the C-bug Category: bug label May 14, 2020
@ehuss
Copy link
Contributor

ehuss commented May 14, 2020

I think the #foo in the URL is ignored in this case. To specify the revision, use the rev = "91a33539" field. And, unfortunately, it appears that is completely undocumented!

@mjarkk
Copy link
Contributor Author

mjarkk commented May 14, 2020

Thanks didn't know this but still wired this isn't specified in the documentation.

@mjarkk mjarkk changed the title Adding a hashtag to the git url in Cargo.toml causes it to show up multiple times in the cargo cli If rev is added to git url cargo gives no warnings and has unexpected behavior May 14, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented May 22, 2020

Is the # syntax required to mean that for all git urls or is it just a github thing?
If it is a git thing, then maybe we should support it.
If it is a github thing, then maybe we should have a warning for urls with both github.com and # recommending the correct syntax.

@ehuss
Copy link
Contributor

ehuss commented May 24, 2020

I can see how that's confusing. It's actually not a github thing, it's a Cargo thing. When Cargo displays a "git source", it displays it in a short format with a hash anchor. GitHub will ignore that completely. GitHub ignores that as a normal URL, but it breaks access with the git protocol.

I'm actually not sure how Cargo should deal with that. Some options off the top of my head:

  • Do nothing, leave it broken. 😜
  • Detect the # fragment, and return a warning or error.
  • Remove the # fragment, otherwise ignore it.
  • Detect the # fragment, and interpret as a git revision (and remove it from the URL).

@ehuss ehuss added the A-git Area: anything dealing with git label May 24, 2020
@mjarkk
Copy link
Contributor Author

mjarkk commented May 24, 2020

Detect the # fragment, and return a warning or error.

I think this is the best option because i guess setting the ref also change other things beside the hash in the URL and because setting the ref is what you are supposed to do.

Remove the # fragment, otherwise ignore it.

Doesn't help the user fixing what they are trying to-do. For example i wanted to add a ref and saw that the cargo output shows the ref in the url so i try to do the same thing, not really helpful if it just gets ignored because now i have to google for a solution.

Detect the # fragment, and interpret as a git revision (and remove it from the URL).

Works but because i guess it also changes other things beside the url hash it will lead new users to believe that this is how git works.

@joshtriplett
Copy link
Member

joshtriplett commented May 27, 2020

We discussed this in today's Cargo meeting, and came to the conclusion that we should notice URLs with fragments in them, warn, and in the warning tell people "if you were trying to specify a specific git revision, use rev = "revision"."

We'd welcome an implementation of that logic.

@joshtriplett joshtriplett added the S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review label May 27, 2020
@mjarkk
Copy link
Contributor Author

mjarkk commented May 28, 2020

can i give making this a try i'm still very new to rust and this would be a nice way to learn a bit more about rust?

@Eh2406
Copy link
Contributor

Eh2406 commented May 28, 2020

@mjarkk Yes, go for it. Feel free to ask here or on discord if we can help!

@mjarkk
Copy link
Contributor Author

mjarkk commented May 30, 2020

I've created a mr to try to fix this #8297

@bors bors closed this as completed in 40ebd52 Jun 1, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 4, 2020
Update cargo

9 commits in 9fcb8c1d20c17f51054f7aa4e08ff28d381fe096..40ebd52206e25c7a576ee42c137cc06a745a167a
2020-05-25 16:25:36 +0000 to 2020-06-01 22:35:00 +0000
- Warn if using hash in git URL, Fixes rust-lang/cargo#8241 (rust-lang/cargo#8297)
- reset lockfile information between resolutions (rust-lang/cargo#8274)
- Disable strip_works test on macos. (rust-lang/cargo#8301)
- Fix typo in impl Display for Strip (rust-lang/cargo#8299)
- Add support for rustdoc root URL mappings. (rust-lang/cargo#8287)
- Fix tests with enoent error message on non-english systems. (rust-lang/cargo#8295)
- Fix fingerprinting for lld on Windows with dylib. (rust-lang/cargo#8290)
- Fix a typo (rust-lang/cargo#8289)
- Fix several issues with close_output test. (rust-lang/cargo#8286)
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 C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants