Strip registry root from `file` in `Index::add()` unconditionally #81

twistedfall · 2025-02-10T19:39:41Z

Hi!

That PR removes the .is_relative() check from Index::add(). The cause for this is that in my tests when started with the relative path as registry root (e.g. cargo-http-registry crates-local publishing crates to such registry fails with:

error: failed to publish to registry at http://127.0.0.1:34245

Caused by:
  the remote server responded with an error: failed to add crates-local/3/t/ttt to git repository, failed to add file to git index, could not find '/home/user/projects/crates-local/crates-local/3/t/ttt' to stat: No such file or directory; class=Os (2); code=NotFound (-3)

So the crates-local is duplicated. This is caused by the fact that when constructing crate_meta_path the base is index root (crates-local) which makes the whole crate_meta_path also relative. Then adding it fails because the stripping of the index root is not performed.

An alternative solution could probably be to avoid calling Index::add with the paths that contain index root, but I haven't investigated this too much.

d-e-s-o · 2025-02-11T03:43:07Z

Hi, thanks for the pull request! Can you please add a test case for the problem being fixed?

twistedfall · 2025-02-11T07:16:53Z

I'll do that, but I have since thought of a corner case that this PR actually breaks. If your registry root dir is called config.json and you use the relative path when you run the server cargo-http-registry config.json, which breaks of course.

twistedfall · 2025-02-11T12:48:03Z

I have changed the approach to only pass relative paths to Index::add() and added the tests for the use-case of passing relative registry root dir

tests/end-to-end.rs

d-e-s-o · 2025-02-11T13:09:04Z

tests/end-to-end.rs

+    RegistryRootPath::Relative => {
+      let root = tempdir().unwrap().into_path();
+      let base_dir = root.parent().unwrap();
+      env::set_current_dir(base_dir).unwrap();


Why do we need to change the current working directory everywhere? This is a nightmare in a multi-threaded context. Plus, if I remove the call everything is fine...

This is needed to make passing relative directory to Index::new() work inside the temporary directory. If you remove this then it the index root will be re-created by the Index::new() in the directory where you run the test command, i.e. project root dir most likely

I don't get it. I don't see anything in Index::new relying on the current working directory. If there is something, that's probably a bug. But sprinkling set_current_dir here is not a fix. Again, tests run concurrently and will trample over each other's set_current_dir calls.

As soon as you pass a relative path in root argument to Index::new() then everything starts depending on the current directory :) E.g. if we pass a literal "index-root" as root argument then for example Index::ensure_config() will write config.json to "index-root/config.json" which effectively writes to "<CWD>/index-root/config.json".

I understand your concerns, but I don't really see an easier way to test that relative paths work while still remaining constrained to the temporary directory.

Ultimately I don't mind dropping this test.

For that to work we still need to change the current directory into the temporary area before calling fs::canonicalize(), otherwise it will resolve to a directory under project root instead of temp dir.

Yeah, I realized that as well. But I still think we should be able to pass in a suitably crafted relative path. E.g., if cargo-http-registry is at /home/user/cargo-http-registry and tempdir creates /tmp/.tmpYb5HBA/, the relative path should be ../../../tmp/.tmpYb5HBA/. I would guess that would be sufficient for testing? Or do we specifically need something without parent directory involvement?

(that may not work for Windows, but that's acceptable)

I though about this, but was hesitating to bring another dep (e.g. pathdiff). Additionally I think it won't always conceptually work on Windows if temp dir and project dir are on the different drives (e.g. C: and D:).

Yes, I understand the Windows restriction, but I think it's fine given that it's only for testing. Same for the dependency: not a problem if it's a dev-only thing (had no idea this functionality was not provided by the standard library though, ugh...).

I think the relative-path avenue would be my preference, though spawning an async runtime in each forked test is fine as well.

That being said, none of the tests fail if I revert the changes to src/index.rs, so I don't know what they test, but it ain't what you set out to fix, I guess...

The changes in index.rs don't really affect anything now that I changed all invocations of Index::add to only use relative paths. If you roll back both index.rs and publish.rs changes you'll get failure in publish_relative_index_root() test.

The test now uses pathdiff to calculate the relative path without setting the current working directory.

Cargo.toml

d-e-s-o

Seems fine to me now, thanks! Left a few more minor comments. Also, can you please squash into a single commit?

d-e-s-o · 2025-02-13T02:03:11Z

src/publish.rs

    format!(
      "failed to add {} to git repository",
      crate_meta_path.display()
    )
  })?;
  index
-    .add(&crate_path)
+    .add(&crate_relative_path)
    .with_context(|| format!("failed to add {} to git repository", crate_path.display()))?;


Suggested change

.with_context(|| format!("failed to add {} to git repository", crate_path.display()))?;

.with_context(|| format!("failed to add {} to git repository", crate_relative_path.display()))?;

Well, I specifically left it so because it would be better to see the full path in the error message for debugging. Maybe display both?

d-e-s-o · 2025-02-13T02:03:50Z

src/publish.rs

@@ -296,14 +297,14 @@ pub fn publish_crate(mut body: Bytes, index: &mut Index) -> Result<()> {
    .write(&data)
    .with_context(|| format!("failed to write to crate file {}", crate_path.display()))?;

-  index.add(&crate_meta_path).with_context(|| {
+  index.add(&crate_meta_relative_path).with_context(|| {
    format!(
      "failed to add {} to git repository",
      crate_meta_path.display()


Suggested change

crate_meta_path.display()

crate_meta_relative_path.display()

src/publish.rs

twistedfall · 2025-02-13T08:08:49Z

Squashed and tended to the comments. I've added both full and relative path to the context for now, but I can remove the full path if you think it shouldn't be there.

d-e-s-o · 2025-02-13T16:37:38Z

Seems fine to me now, thanks!

Add a CHANGELOG entry for pull request #81. Also merge the publish and publish_relative_index_root test cases into a single one, to avoid unnecessary code duplication.

…ve paths to it This fixes being unable to use relative paths when running the server and also handles some corner cases like having the index root dir named as `config.json`.

twistedfall force-pushed the fix-repo-root-handling branch from 843950d to e79fc79 Compare February 11, 2025 12:47

d-e-s-o reviewed Feb 11, 2025

View reviewed changes

twistedfall requested a review from d-e-s-o February 11, 2025 13:26

d-e-s-o approved these changes Feb 13, 2025

View reviewed changes

twistedfall force-pushed the fix-repo-root-handling branch from d70df31 to cdb428b Compare February 13, 2025 08:07

d-e-s-o merged commit 42a6be9 into d-e-s-o:main Feb 13, 2025
6 checks passed

d-e-s-o added a commit that referenced this pull request Feb 13, 2025

Add CHANGELOG entry for #81

d70b313

Add a CHANGELOG entry for pull request #81. Also merge the publish and publish_relative_index_root test cases into a single one, to avoid unnecessary code duplication.

d-e-s-o added a commit that referenced this pull request Feb 13, 2025

Add CHANGELOG entry for #81

4eb3fd2

Add a CHANGELOG entry for pull request #81. Also merge the publish and publish_relative_index_root test cases into a single one, to avoid unnecessary code duplication.

Stop prefix-stripping file in Index::add() and always pass relati…

cdb428b

…ve paths to it This fixes being unable to use relative paths when running the server and also handles some corner cases like having the index root dir named as `config.json`.

twistedfall deleted the fix-repo-root-handling branch February 14, 2025 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip registry root from `file` in `Index::add()` unconditionally #81

Strip registry root from `file` in `Index::add()` unconditionally #81

twistedfall commented Feb 10, 2025

d-e-s-o commented Feb 11, 2025

twistedfall commented Feb 11, 2025

twistedfall commented Feb 11, 2025

d-e-s-o Feb 11, 2025

twistedfall Feb 11, 2025

d-e-s-o Feb 11, 2025

twistedfall Feb 11, 2025

twistedfall Feb 11, 2025

d-e-s-o Feb 11, 2025 •

edited

Loading

twistedfall Feb 11, 2025

d-e-s-o Feb 11, 2025

twistedfall Feb 11, 2025

twistedfall Feb 12, 2025

d-e-s-o left a comment

d-e-s-o Feb 13, 2025

twistedfall Feb 13, 2025

d-e-s-o Feb 13, 2025

twistedfall commented Feb 13, 2025

d-e-s-o commented Feb 13, 2025

	.with_context(\|\| format!("failed to add {} to git repository", crate_path.display()))?;
	.with_context(\|\| format!("failed to add {} to git repository", crate_relative_path.display()))?;

Strip registry root from file in Index::add() unconditionally #81

Strip registry root from file in Index::add() unconditionally #81

Conversation

twistedfall commented Feb 10, 2025

d-e-s-o commented Feb 11, 2025

twistedfall commented Feb 11, 2025

twistedfall commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-e-s-o Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-e-s-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twistedfall commented Feb 13, 2025

d-e-s-o commented Feb 13, 2025

Strip registry root from `file` in `Index::add()` unconditionally #81

Strip registry root from `file` in `Index::add()` unconditionally #81

d-e-s-o Feb 11, 2025 •

edited

Loading