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.

Sign up for GitHub

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

Jump to bottom

Support path_in_vcs as part of cargo_vcs_metadata #9866

Merged
merged 1 commit into from
Sep 26, 2021
Merged

Support path_in_vcs as part of cargo_vcs_metadata #9866

merged 1 commit into from
Sep 26, 2021

Conversation

nipunn1313
Copy link
Contributor

Depends on #9865 - this PR will look simpler once that one lands. I can't target my PR to the base branch of #9865 as an external contributor (since it's in my fork). For now just review the latest commit of this one.

This is a backward compatible change that is an alternative to #9837

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2021
@nipunn1313
Copy link
Contributor Author

This is an API change - so I assume we'd want to go through some discussion/review to confirm it's reasonable

@nipunn1313 nipunn1313 mentioned this pull request Sep 2, 2021
Closed
@kornelski
Copy link
Contributor

kornelski commented Sep 2, 2021 •
edited
Loading

This is very useful for rendering crate's READMEs correctly.

@nipunn1313 nipunn1313 force-pushed the pkg_in_repo2 branch 3 times, most recently from aac20b6 to 94e4f0d Compare September 2, 2021 19:19
@nipunn1313
Copy link
Contributor Author

(Updated documentation as well here)

@nipunn1313 nipunn1313 mentioned this pull request Sep 3, 2021
Closed
@alexcrichton
Copy link
Member

Seems like a good change to me! I'll let @ehuss weigh in though as well.

@ehuss
Copy link
Contributor

ehuss commented Sep 26, 2021

Sorry for the delay. There's a lot of comments to sift through here, can you maybe put together a summary? I'm a bit confused how the repository root comes into play. Doesn't it need to know the path of the README relative to the package root? I don't see how the repository root is related to that.

@nipunn1313
Copy link
Contributor Author

Sure! I'll try to address the need in a summary.

Original Issue rust-lang/crates.io#3484

Relative links in README files which aren't in the repository root get rendered with incorrect links.

Crates.io rewrites READMEs inside of github/bitbucket/gitlab repos so that the relative markdown links actually link to the page on github/bitbucket/gitlab. Unfortunately, crates.io did this by calculating (repo_url + rel_path) - assuming that the README was in the root of the repo. Thus READMEs outside the root of the repo aren't handled.

Partial fix

rust-lang/crates.io#3861 - partially fixes this by using the readme_path to calculate URLs as (repo_root + parent(readme_path) + rel_path).

This still does not handle cases where the package is in a subdirectory - as readme_path is relative to the package.

Full fix

Cargo needs to provide enough information such that crates.io (or other providers) can calculate the full URL to links.

With this diff, the approach is to add a path_in_vcs to the package metadata, such that crates.io can calculate
(repo_root + path_in_vcs + parent(readme_path) + rel_path)

#9837 had the non-backward-compatible approach of changing readme_path to be repo-root-relative. My sense is that all folks prefer this strategy to that one.

@ehuss
Copy link
Contributor

ehuss commented Sep 26, 2021

I see, that makes a lot more sense to me, thank you!

Thanks for updating the docs. Just one minor request, can the details about the .cargo_vcs_info.json format be moved up to the Description section? The Examples section is intended for short blurbs for people who are mostly unfamiliar with cargo, and that audience doesn't need to know about this file. I would just add a level-3 sub-section header at the bottom of the Description section that describes the format of the file. I think a single example would be sufficient, with some comments similar to the cargo-metadata.md file, it can use the javascript language to get highlighting to work correctly.

@ehuss ehuss added the T-cargo Team: Cargo label Sep 26, 2021
@ehuss
Copy link
Contributor

ehuss commented Sep 26, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2021

📌 Commit b8b127a has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2021
@bors
Copy link
Contributor

bors commented Sep 26, 2021

⌛ Testing commit b8b127a with merge cc8eb13...

@bors
Copy link
Contributor

bors commented Sep 26, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing cc8eb13 to master...

@bors bors merged commit cc8eb13 into rust-lang:master Sep 26, 2021
@nipunn1313 nipunn1313 deleted the pkg_in_repo2 branch September 26, 2021 22:41
@ehuss ehuss mentioned this pull request Sep 29, 2021
Merged
ehuss added a commit to ehuss/rust that referenced this pull request Sep 30, 2021
@ehuss
Rollup merge of rust-lang#89377 - ehuss:update-cargo, r=ehuss
4bb4789 
Update cargo

5 commits in 0121d66aa2ef5ffa9735f86c2b56f5fdc5a837a6..d56b42c549dbb7e7d0f712c51b39400260d114d4
2021-09-22 16:08:27 +0000 to 2021-09-27 13:44:18 +0000
- Allow `cargo update --precise` with metadata. (rust-lang/cargo#9945)
- Support path_in_vcs as part of cargo_vcs_metadata (rust-lang/cargo#9866)
- Doc about InstallTracker files and `install.root` (rust-lang/cargo#9948)
- Add some clarity on the license/license-file warning. (rust-lang/cargo#9941)
- Fix the problem that help cannot be displayed properly (rust-lang/cargo#9933)
nipunn1313 added a commit to nipunn1313/crates.io that referenced this pull request Oct 1, 2021
@nipunn1313
Extract cargo vcs info from tarball
01b8537 
Supported in rust-lang/cargo#9866
nipunn1313 added a commit to nipunn1313/crates.io that referenced this pull request Oct 1, 2021
@nipunn1313
controllers::krate::publish: Extract cargo vcs info from tarball
b8978f4 
Supported in rust-lang/cargo#9866
@nipunn1313 nipunn1313 mentioned this pull request Oct 1, 2021
Merged
nipunn1313 added a commit to nipunn1313/crates.io that referenced this pull request Oct 28, 2021
@nipunn1313
controllers::krate::publish: Extract cargo vcs info from tarball
ed10fa8 
Supported in rust-lang/cargo#9866
nipunn1313 added a commit to nipunn1313/crates.io that referenced this pull request Oct 28, 2021
@nipunn1313
controllers::krate::publish: Extract cargo vcs info from tarball
52d9d24 
Supported in rust-lang/cargo#9866
nipunn1313 added a commit to nipunn1313/crates.io that referenced this pull request Oct 30, 2021
@nipunn1313
controllers::krate::publish: Extract cargo vcs info from tarball
251e6e6 
Supported in rust-lang/cargo#9866
Turbo87 pushed a commit to nipunn1313/crates.io that referenced this pull request Jan 3, 2022
@nipunn1313 @Turbo87
controllers::krate::publish: Extract cargo vcs info from tarball
8350593 
Supported in rust-lang/cargo#9866
Turbo87 pushed a commit to nipunn1313/crates.io that referenced this pull request Jan 3, 2022
@nipunn1313 @Turbo87
controllers::krate::publish: Extract cargo vcs info from tarball
86d852f 
Supported in rust-lang/cargo#9866
bors added a commit to rust-lang/crates.io that referenced this pull request Jan 3, 2022
@bors
Auto merge of #3973 - nipunn1313:readme2, r=Turbo87
3f67acf 
Support relative URLs from repo-subdirectory packages

Extract `.cargo_vcs_info.json` from the tarball and use it to determine the path in vcs.

Support for this was added to cargo with
rust-lang/cargo#9866

Fixes #3484
(note that support for this in admin::render_readmes not yet exists - would come with #4095)

Depends on #4097 (Because of community/community#4477 - github ends up rendering the diffs together. Go to the commits tab and just look at the most recent commits to review).
@ehuss ehuss added this to the 1.57.0 milestone Feb 6, 2022
@epage epage mentioned this pull request Apr 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nipunn1313 @rust-highfive @kornelski @alexcrichton @ehuss @bors