Skip to content

Implement parseURLRelative, use in HttpBinaryCacheStore#13819

Merged
xokdvium merged 1 commit intoNixOS:masterfrom
obsidiansystems:relative-url
Aug 27, 2025
Merged

Implement parseURLRelative, use in HttpBinaryCacheStore#13819
xokdvium merged 1 commit intoNixOS:masterfrom
obsidiansystems:relative-url

Conversation

@Ericson2314
Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 commented Aug 24, 2025

Motivation

This allows us to replace some very hacky and not correct string concatentation in HttpBinaryCacheStore.

Also make fixGitURL returned a ParsedURL.

Context

It will especially be useful with #13752, when today's hacks started to cause problems in practice, not just theory.


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner August 24, 2025 18:36
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Aug 24, 2025
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

Some minor notes. Would be great to have a bit more tests but otherwise LGTM. The merging algorithm should do exactly what we want for this logic: https://datatracker.ietf.org/doc/html/rfc3986#section-5.2.2.

@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Aug 25, 2025
@Ericson2314 Ericson2314 force-pushed the relative-url branch 2 times, most recently from 451aa6f to cf3f40c Compare August 25, 2025 20:50
/* Hack: Boost `url_view` supports Zone IDs, but `url` does not.
Just manually take the authority from the original URL to work
around it. See https://github.com/boostorg/url/issues/919 for
details. */
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Ericson2314 Ericson2314 force-pushed the relative-url branch 2 times, most recently from 0d540fc to 03637d4 Compare August 25, 2025 22:08
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

Minor notes

This allows us to replace some very hacky and not correct string
concatentation in `HttpBinaryCacheStore`. It will especially be useful
with NixOS#13752, when today's hacks started to cause problems in practice,
not just theory.

Also make `fixGitURL` returned a `ParsedURL`.
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

LGTM in general.

@xokdvium xokdvium merged commit 8ee7479 into NixOS:master Aug 27, 2025
24 of 25 checks passed
@Ericson2314 Ericson2314 deleted the relative-url branch August 27, 2025 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fetching Networking with the outside (non-Nix) world, input locking store Issues and pull requests concerning the Nix store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants