Submit repo-relative readme path relative to crates.io #9837

nipunn1313 · 2021-08-24T06:33:29Z

Ideally it's a repo-relative readme path, but this seems
to be the closest we can get. It's typical to have a workspace
be at the repo root, so it seems like an improvement at least.

Crates.io (currently) uses the readme path to determine its filetype.
With this, crates.io would
also use it to render relative links in non-root READMEs. This diff would allow
it to correctly render relative links in READMEs from non-root Cargo.toml

Added unit tests for publishing with

cargo.toml in root + readme in root
cargo.toml in root + readme in subdir
cargo.toml in subdir + readme next to it
cargo.toml in subdir + readme in root (../README)

Should fix rust-lang/crates.io#3484 in
combination with rust-lang/crates.io#3861

rust-highfive · 2021-08-24T06:33:32Z

r? @alexcrichton

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

nipunn1313 · 2021-08-24T06:43:36Z

Here's my screenshot of an e2e test where I use my local cargo (w/ this patch) to cargo publish to my local crates.io with rust-lang/crates.io#3861

As you can see, the relative link to Otherfile.md is appropriately inside the innercrate/docs directory! (statusquo is a broken link to just README)

nipunn1313 · 2021-08-24T08:10:59Z

I realized I could just do repo-relative paths via git2::discover() - so I did that!
What's the recommended way to get this working on windows? Convert a PathBuf to a fwd-slash path (since that's I think what crates.io expects).

nipunn1313 · 2021-08-24T08:17:56Z

src/cargo/ops/registry.rs

+
+    let readme_path_relative_to_repo = readme.as_ref().map(|readme| {
+        || -> Option<String> {
+            let repo = git2::Repository::discover(pkg.root()).ok()?;


would this ever problematically discover too high up (eg maybe user made their whole root a git directory, and are working on a cargo project that isn't in git)? Is there some way to bound this discovery? Maybe we don't care so much for that case. It seems very contrived.

Oh I see now my comment below is related to this. tl;dr; we already do some form of git repo discovery so this should match that (either by threading info or with a helper function). I think this is a good question to ask but it's a preexisting one you don't need to solve here.

nipunn1313 · 2021-08-24T08:20:40Z

tests/testsuite/publish.rs

+          "links": null,
+          "name": "foo",
+          "readme": "HELLO",
+          "readme_file": "docs/README",


this will probably end up failing on windows with a backslash here? I think we'd want to send forwardslashes to crates.io here. How to convert?

With green tests I think this worked out?

I think I need to write a new test to exercise this situation - where the Cargo.toml is not in the root of the repo (eg a repo with multiple crates).

I would expect it to come up with something like
path\to\innercrate\docs/README as is

Ah ok that makes sense, I think it's fine to specifically change \ to / with a replace call in an ad-hoc fashion since it's purely for this one field.

Cool. I was unable to get the tests to actually cover this because they use weak json matching (that converts \ to /). I made the replace change and manually confirmed it works with some println's in the compare.rs to make sure.

I wonder if there's somewhere we can put some assert in the test implementation of the registry. I was having trouble tracking down what cargo publish does in the unit tests - maybe the test backend can assert for fwd slashes only? Do you know where to look for that?

alexcrichton · 2021-08-27T19:38:11Z

Seems reasonable to me to add, thanks for this!

For finding the git repo, IIRC we do that somewhere in the packaging step to get the head revision to encode in some metadata we upload as well. Instead of rediscovering the repository twice could the repo information get threaded out of there into this transmission step? I'm slightly worried that the parent git repo isn't always guaranteed to be the project repo so we may add heuristics one day to improve that logic, and if we do so we should ideally only do that in one location.

Added unit tests for publishing with - cargo.toml in root + readme in root - cargo.toml in root + readme in subdir - cargo.toml in subdir + readme next to it - cargo.toml in subdir + readme in root (../README) Should fix rust-lang/crates.io#3484 in combination with rust-lang/crates.io#3861

nipunn1313 · 2021-08-28T06:12:09Z

For finding the git repo, IIRC we do that somewhere in the packaging step to get the head revision to encode in some metadata we upload as well. Instead of rediscovering the repository twice could the repo information get threaded out of there into this transmission step? I'm slightly worried that the parent git repo isn't always guaranteed to be the project repo so we may add heuristics one day to improve that logic, and if we do so we should ideally only do that in one location.

Started doing this and realized it was a bit of a refactor - so I split it out into its own PR - #9851. That PR depends on this one - so github's diff view isn't great about only showing the differences. But if this one approves and lands, I think github should update #9851 to only show the refactor portion.

alexcrichton · 2021-08-30T14:31:16Z

@bors: r+

bors · 2021-08-30T14:31:17Z

📌 Commit bb7a63f has been approved by alexcrichton

ehuss · 2021-08-30T14:42:20Z

@bors r-

Sorry, I'd like to make some comments here.

ehuss · 2021-08-30T14:46:19Z

Has #5911 or #8434 been considered here? Would this make it more difficult for other registries to fix the relative links?

cc @kornelski, can you comment on whether or not lib.rs would be able to make use of this or have problems?

At a minimum, the documentation needs to be updated here.

Do you know if it will break other registries? This is also somewhat of a breaking change, since it is changing the meaning of readme_file.

This seems to make readme_file contain paths that don't exist in the .crate file, making them possibly even more difficult to handle.

kornelski · 2021-08-30T16:10:46Z

Making readme_file point to a file that is not in .crate tarball is a breaking change. I'd prefer to keep this field as-is.

How about extending .cargo_vcs_info.json to have a key like path_in_repository?

For lib.rs I'm cloning the repo URL from repository Cargo.toml field, and recursively search the cloned repo for the crate in order to discover the crate's path inside the repo. If you could provide the path in repo via the crate tarball it'd be very helpful.

nipunn1313 · 2021-08-30T20:02:13Z

Hey! Thanks @ehuss for linking those issues. It didn't come across my mind that there are other registry implementations.

I had read through the crates.io implementation, and it only uses the extension of the readme file - so it wouldn't have an effect there. Crates.io doesn't really use the full path. However, if this is an API change used by multiple registry implementations, then it is in fact a breaking change.

rust-lang/crates.io#3861 would get crates.io to rewrite the relative links.

It looks like @kornelski had some nice writeup in #8434 which overlaps rust-lang/crates.io#3484.
I'll leave comments in #8434 to keep them together

nipunn1313 · 2021-09-02T06:18:17Z

How about extending .cargo_vcs_info.json to have a key like path_in_repository?

For lib.rs I'm cloning the repo URL from repository Cargo.toml field, and recursively search the cloned repo for the crate in order to discover the crate's path inside the repo. If you could provide the path in repo via the crate tarball it'd be very helpful.

I implemented this idea in #9866. It looks pretty elegant - and backward compatible!

nipunn1313 · 2021-09-09T18:28:03Z

going to close in favor of #9866

Support path_in_vcs as part of cargo_vcs_metadata 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 assigned alexcrichton Aug 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2021

nipunn1313 mentioned this pull request Aug 24, 2021

Relative URLs in subdirectory readmes rust-lang/crates.io#3484

Closed

nipunn1313 force-pushed the readme_path branch from c20b768 to 0be94af Compare August 24, 2021 08:09

nipunn1313 force-pushed the readme_path branch from 0be94af to 7efb87e Compare August 24, 2021 08:14

nipunn1313 commented Aug 24, 2021

View reviewed changes

nipunn1313 changed the title ~~Submit ws-relative readme path relative to crates.io~~ Submit repo-relative readme path relative to crates.io Aug 24, 2021

nipunn1313 commented Aug 24, 2021

View reviewed changes

nipunn1313 force-pushed the readme_path branch 2 times, most recently from 167f947 to 32a8c7a Compare August 28, 2021 05:28

nipunn1313 force-pushed the readme_path branch from 32a8c7a to bb7a63f Compare August 28, 2021 05:29

nipunn1313 mentioned this pull request Aug 28, 2021

Refactor so package publishing only determines git root in one place #9851

Closed

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 Aug 30, 2021

bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2021

This was referenced Aug 30, 2021

Fixup of repository-relative URLs in READMEs #8434

Closed

Support path_in_vcs as part of cargo_vcs_metadata #9866

Merged

nipunn1313 closed this Sep 9, 2021

nipunn1313 deleted the readme_path branch September 26, 2021 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submit repo-relative readme path relative to crates.io #9837

Submit repo-relative readme path relative to crates.io #9837

nipunn1313 commented Aug 24, 2021

rust-highfive commented Aug 24, 2021

nipunn1313 commented Aug 24, 2021

nipunn1313 commented Aug 24, 2021

nipunn1313 Aug 24, 2021

alexcrichton Aug 27, 2021

nipunn1313 Aug 24, 2021

alexcrichton Aug 27, 2021

nipunn1313 Aug 27, 2021

alexcrichton Aug 27, 2021

nipunn1313 Aug 28, 2021

alexcrichton commented Aug 27, 2021

nipunn1313 commented Aug 28, 2021

alexcrichton commented Aug 30, 2021

bors commented Aug 30, 2021

ehuss commented Aug 30, 2021

ehuss commented Aug 30, 2021

kornelski commented Aug 30, 2021 •

edited

Loading

nipunn1313 commented Aug 30, 2021

nipunn1313 commented Sep 2, 2021

nipunn1313 commented Sep 9, 2021

Submit repo-relative readme path relative to crates.io #9837

Submit repo-relative readme path relative to crates.io #9837

Conversation

nipunn1313 commented Aug 24, 2021

rust-highfive commented Aug 24, 2021

nipunn1313 commented Aug 24, 2021

nipunn1313 commented Aug 24, 2021

nipunn1313 Aug 24, 2021

Choose a reason for hiding this comment

alexcrichton Aug 27, 2021

Choose a reason for hiding this comment

nipunn1313 Aug 24, 2021

Choose a reason for hiding this comment

alexcrichton Aug 27, 2021

Choose a reason for hiding this comment

nipunn1313 Aug 27, 2021

Choose a reason for hiding this comment

alexcrichton Aug 27, 2021

Choose a reason for hiding this comment

nipunn1313 Aug 28, 2021

Choose a reason for hiding this comment

alexcrichton commented Aug 27, 2021

nipunn1313 commented Aug 28, 2021

alexcrichton commented Aug 30, 2021

bors commented Aug 30, 2021

ehuss commented Aug 30, 2021

ehuss commented Aug 30, 2021

kornelski commented Aug 30, 2021 • edited Loading

nipunn1313 commented Aug 30, 2021

nipunn1313 commented Sep 2, 2021

nipunn1313 commented Sep 9, 2021

kornelski commented Aug 30, 2021 •

edited

Loading