Skip to content

Handle empty ports with new URL parsing#13832

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
kip93:fix/empty-ports
Aug 26, 2025
Merged

Handle empty ports with new URL parsing#13832
Ericson2314 merged 1 commit intoNixOS:masterfrom
kip93:fix/empty-ports

Conversation

@kip93
Copy link
Copy Markdown
Contributor

@kip93 kip93 commented Aug 26, 2025

Motivation

Fix #13830

Context

@sbc64 mentioned he had an issue with the new URL handling and it looked simple to fix, so I took the liberty of doing so.

Add 👍 to pull requests you find important.

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

@kip93 kip93 requested a review from edolstra as a code owner August 26, 2025 15:56
@xokdvium
Copy link
Copy Markdown
Contributor

xokdvium commented Aug 26, 2025

Thanks, you beat me to the fix!

One thing to consider with this is that new lock files will be slightly different. Old parsing didn't drop the empty port:

git+ssh://git@github.com:/NixOS/nix

But new logic drops the colon:

git+ssh://git@github.com/NixOS/nix

The new behavior is compliant with the RFC (https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.3):

URI producers and
normalizers should omit the port component and its ":" delimiter if
port is empty

But I'm pretty sure that the canonicalization shim here effectively solves this issue and newer nix versions will not consider the lock file outdated (note the to_string call).

nix/src/libflake/flakeref.cc

Lines 290 to 300 in 1e16a54

if (auto url = fetchers::maybeGetStrAttr(flakeRef.input.attrs, "url")) {
try {
auto parsed = parseURL(*url, /*lenient=*/true);
if (auto dir2 = get(parsed.query, "dir")) {
if (flakeRef.subdir != "" && flakeRef.subdir == *dir2)
parsed.query.erase("dir");
}
flakeRef.input.attrs.insert_or_assign("url", parsed.to_string());
} catch (BadURL &) {
}
}

@kip93
Copy link
Copy Markdown
Contributor Author

kip93 commented Aug 26, 2025

Interesting, yes we can confirm that if I do a lock with this PR and then run nix flake lock using 2.30 it tried to update the lock, but the other way around it's correctly left untouched.

@xokdvium xokdvium added the backport 2.31-maintenance Automatically creates a PR against the branch label Aug 26, 2025
@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented Aug 26, 2025

But I'm pretty sure that the canonicalization shim here effectively solves this issue and newer nix versions will not consider the lock file outdated (note the to_string call).

Even if they did, it is easy to just remove the : from a flake.nix, so all Nix versions agree on the normal form that should be in the flake.lock.

@Ericson2314 Ericson2314 merged commit 0bd9d6a into NixOS:master Aug 26, 2025
14 checks passed
@kip93 kip93 deleted the fix/empty-ports branch August 26, 2025 17:56
@mergify mergify bot mentioned this pull request Aug 26, 2025
Merged
mergify bot added a commit that referenced this pull request Aug 26, 2025
@mergify
Merge pull request #13834 from NixOS/mergify/bp/2.31-maintenance/pr-1…
7aa0aca 
…3832

Handle empty ports with new URL parsing (backport #13832)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees

No one assigned

Labels

backport 2.31-maintenance Automatically creates a PR against the branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flake input url parameters fail to be parsed when port is empty

3 participants

@kip93 @xokdvium @Ericson2314