Skip to content

Preserve relative paths in lockfiles for flat indices#15870

Closed
harshithvh wants to merge 7 commits intoastral-sh:mainfrom
harshithvh:hvh/preserve-relative-paths
Closed

Preserve relative paths in lockfiles for flat indices#15870
harshithvh wants to merge 7 commits intoastral-sh:mainfrom
harshithvh:hvh/preserve-relative-paths

Conversation

@harshithvh
Copy link
Copy Markdown
Contributor

Summary

Modified Source::from_index_url to check the original user input and preserve relative paths when the user originally provided them.

  • Preserves user intent for relative paths
  • Maintains backward compatibility for absolute paths

fixes: #15055

Test Plan

added lock_find_links_relative_path_preserved, validates that flat indices preserve relative paths in lockfiles instead of converting them to absolute paths

Copy link
Copy Markdown
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Thank you!

@harshithvh
Copy link
Copy Markdown
Contributor Author

hey @konstin
Can we close this if all looks good?
Thanks!

@konstin
Copy link
Copy Markdown
Member

konstin commented Sep 16, 2025

I want to give @charliermarsh a chance to look at this too, afterwards we can merge it.

@konstin konstin added the bug Something isn't working label Sep 16, 2025
@charliermarsh
Copy link
Copy Markdown
Member

I will review today.

@harshithvh
Copy link
Copy Markdown
Contributor Author

@konstin, all looks good to me
mind giving a final review

if let Ok(verbatim_url) = VerbatimUrl::from_url_or_path(&index_str, None) {
IndexMetadata::from(IndexUrl::from(verbatim_url.with_given(&index_str)))
} else {
// provide a fallback
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In which case does the code above fail but the logic below pass?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it’s redundant. from_url_or_path already accepts both valid URLs and paths; anything DisplaySafeUrl::parse would accept is already accepted by from_url_or_path.

@harshithvh
Copy link
Copy Markdown
Contributor Author

hey @konstin, sorry for keeping this PR hanging. Please review the changes when you have some time

@@ -3582,9 +3582,23 @@ impl Source {
.to_file_path()
.map_err(|()| LockErrorKind::UrlToPath { url: url.to_url() })?;
let path = relative_to(&path, root)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add some documentation around why we need both the relative_to and reading the relative path from given?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added docs! Hope this helps future readers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I still don't follow. When they are on different drives (I assume you mean Windows drive letters, not Unix mounts?), how can the user config use a relative path?

Can you write a test case for this? The current test case passes with only the requirement.rs changes applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Relative indexes are stored as absolute in the lockfile

3 participants