Implement support for percent encoded filepaths for flakerefs #6614

RasmusRendal · 2022-06-03T20:46:46Z

To support using nix flakes in paths with spaces, this introduces the convention that the path part of the URL should be percent-encoded when dealing with paths, and that it's the responsibility of fetchers that use a local path to decode it.

It follows the convention of firefox, which uses percent encoding when encoding a local path to an URL.

Eventually, this might also allow paths with arbitrary unicode characters (#4563, #5759), if the percent encoding and decoding methods are improved such that they can handle them.

This closes #6394.

I can get rid of the hijacked test if you'd prefer it.

src/libutil/url.hh

thufschmitt · 2022-06-14T14:20:49Z

Having another look at it, I'm a bit confused by the semantics: if foo bar is a flake, then nix build "./foo bar" will work (is that intended? You added to the allowed characters in a filepath but it's not documented), but not nix build ./foo%20bar. And conversely, nix build "path:$PWD/foo bar" won't work, but nix build path:$PWD foo%20bar will.

Also, can you add something to the release notes for this?

RasmusRendal · 2022-06-14T15:40:25Z

Having another look at it, I'm a bit confused by the semantics: if foo bar is a flake, then nix build "./foo bar" will work (is that intended? You added to the allowed characters in a filepath but it's not documented), but not nix build ./foo%20bar. And conversely, nix build "path:$PWD/foo bar" won't work, but nix build path:$PWD foo%20bar will.

These are my intended semantics at least. IMHO, users expect command-line tools to work with paths by default, and that is what they should do. If the user supplies something like ./directory, then we should treat is as a directory, and not as a URL. If the user wants to work with URLs instead, then they should provide the prefix i.e. path:$PWD/foo%20bar.

Also, can you add something to the release notes for this?

Done :D

thufschmitt · 2022-06-20T19:09:21Z

These are my intended semantics at least. IMHO, users expect command-line tools to work with paths by default, and that is what they should do. If the user supplies something like ./directory, then we should treat is as a directory, and not as a URL. If the user wants to work with URLs instead, then they should provide the prefix i.e. path:$PWD/foo%20bar.

Yes that kinda makes sense indeed… at least I can think of any better semantics. @edolstra any thought on that?

crasm · 2023-01-31T07:52:49Z

Any update on if this can be merged? I was just hit by #6394 my first time trying to set up a flake-based project dev shell.

thufschmitt · 2023-02-01T17:13:17Z

Any update on if this can be merged?

Yes, I think it can, sorry about the long delay @RasmusRendal.

There are a few conflicts now, do you mind resolving them?

nomeata · 2023-02-28T09:05:16Z

@RasmusRendal, are you still working on this PR or should someone take it over?

RasmusRendal · 2023-02-28T13:31:13Z

@RasmusRendal, are you still working on this PR or should someone take it over?

Yeah I don't think I'll have the time in the immediate future to get into the Nix codebase again. If somebody else would solve the merge conflicts, that would be very nice of them.

thufschmitt · 2023-03-04T12:49:17Z

@RasmusRendal, are you still working on this PR or should someone take it over?

Yeah I don't think I'll have the time in the immediate future to get into the Nix codebase again. If somebody else would solve the merge conflicts, that would be very nice of them.

Ok, thanks for letting us know 👍

Unless someone is willing to beat me to it I can take over that one as it's an important fix (and also probably fixes #5759 which is of the same vein).

thufschmitt · 2023-03-04T12:58:35Z

src/libexpr/flake/flakeref.cc

-    else if (std::regex_match(url, match, pathUrlRegex)) {
-        std::string path = match[1];
-        std::string fragment = percentDecode(match.str(3));
+    else if (pathExists(url.substr(0, url.find("#")))) {


Some notes (for myself mostly) on that change since it's a fairly crucial one:

The pathExists call is potentially problematic because the flakeref should exist independently of what the filesystem contains. Maybe we could just split unconditionnally regardless of whether the path exist

Splitting on # is probably not enough since the syntax of a path flakeref is of the form <path>(\?<params>)?(#<fragments>)?. So we'll want to split on ? too

I guess this isn't truly correct if multi-byte utf8 glyphs can contain \63 (the ascii code for ?) since we'd just split here. I think this isn't the case (precisely to allow this kind of things), but even if it's the case, it's probably better than the current situation

if multi-byte utf8 glyphs can contain \63

I don't think they can, utf8 is carefully defined that the ascii bytes are only used for the coresponding character. All the other stuff has the highest bit set

thufschmitt · 2023-04-20T06:04:44Z

I've updated this. @NixOS/nix-team, can someone have a look?

The actual change is 1c6d451, with 2b16210 being some extra cleanup that can be moved to a separate PR if needed (and the rest being tests and release notes).

fricklerhandwerk

This may need some user-visible documentation in the flake references section. One sentence and an example should be enough.

doc/manual/src/release-notes/rl-next.md

thufschmitt · 2023-05-09T08:09:18Z

This may need some user-visible documentation in the flake references section. One sentence and an example should be enough.

I started giving it a shot, but it turns out that the documentation for plain path flakerefs (/foo/bar or ./foo/bar) is totally wrong already and would need to be rewritten. I've opened #8308 to track it.

Since rewriting that doc would have a slightly bigger scope, I'd be in favor of merging this PR first and fixing the docs later. Does that sound good?

fricklerhandwerk · 2023-05-09T10:31:37Z

Yeah, I can live with a lower standard of documentation quality for experimental features.

thufschmitt · 2023-07-25T16:19:37Z

Rebased to fix a couple benign conflicts. With #8640 being out of the way, this one should be good to go

TheLostLambda · 2023-08-28T10:06:17Z

Would love to get this rebased and merged! The bug described in #6394 with spaces caused me quite the headache this morning!

I'm somewhat new to Nix and loving it so far, but it's little things like this that really drive people away when the point of a tool is automating away other tooling problems!

thufschmitt · 2023-09-05T07:53:10Z

@fricklerhandwerk would you care to give this another look?

src/nix/flake.md

doc/manual/src/release-notes/rl-next.md

src/nix/flake.md

fricklerhandwerk

Sorry for the back and forth, but it was really unclear to me what this changes. If I understand it correctly now, we can simplify a lot.

doc/manual/src/release-notes/rl-next.md

src/nix/flake.md

Support using nix flakes in paths with spaces or abitrary unicode characters. This introduces the convention that the path part of the URL should be percent-encoded when dealing with `path:` urls and not when using filepaths (following the convention of firefox). Co-authored-by: Rendal <rasmus@rend.al>

Was starting to be very complex and hard to follow. Now the different cases should be easier to understand.

thufschmitt · 2023-09-22T08:08:41Z

@fricklerhandwerk I gave the documentation and release notes another pass, PTAL.

If that's still not clear, I'd like to take ½h or 1h to pair with you on it so that we can get this through and not have it wait forever

fricklerhandwerk

Thanks a lot, makes sense now. Slight grammatical fixes, otherwise good to merge.

src/nix/flake.md

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

crasm · 2023-09-25T23:35:13Z

Might just be a CI issue? Quite literally ran for an hour.

2023-09-25T07:57:06.1198552Z Requested labels: ubuntu-latest
[...]
2023-09-25T08:56:32.3792330Z building '/nix/store/jifwp7plfdh9kxqy30619k6j0a7py2n3-install-tests.drv'...
2023-09-25T08:57:25.0614911Z ##[error]The operation was canceled.

RasmusRendal force-pushed the spaces branch from 1939bd1 to fb29a00 Compare June 4, 2022 11:17

liketechnik mentioned this pull request Jun 4, 2022

path: or file: flake URLs containing whitespace do not work NixOS/nixpkgs#171821

Closed

RasmusRendal force-pushed the spaces branch from fb29a00 to 4e5a0f2 Compare June 4, 2022 17:26

edolstra reviewed Jun 7, 2022

View reviewed changes

src/libutil/url.hh Outdated Show resolved Hide resolved

src/libutil/url.hh Outdated Show resolved Hide resolved

RasmusRendal force-pushed the spaces branch 2 times, most recently from 274af6c to 654927c Compare June 7, 2022 19:22

thufschmitt approved these changes Jun 8, 2022

View reviewed changes

RasmusRendal requested a review from edolstra June 11, 2022 10:16

RasmusRendal force-pushed the spaces branch from fed3591 to a532d74 Compare July 12, 2022 13:17

thufschmitt mentioned this pull request Feb 22, 2023

Properly escape local paths into URLs in fetchTree #7776

Merged

7 tasks

fricklerhandwerk added flakes language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Mar 3, 2023

thufschmitt mentioned this pull request Mar 3, 2023

[Flake] error: 'file:///tmp/测试' is not a valid URL #5759

Open

thufschmitt reviewed Mar 4, 2023

View reviewed changes

thufschmitt force-pushed the spaces branch from a532d74 to 2b16210 Compare April 20, 2023 06:02

thufschmitt requested a review from fricklerhandwerk as a code owner April 20, 2023 06:02

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 20, 2023

fricklerhandwerk reviewed May 3, 2023

View reviewed changes

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved

thufschmitt mentioned this pull request May 9, 2023

The documentation for plain path flakerefs (/foo or ./foo) isn't correct #8308

Closed

2 tasks

thufschmitt marked this pull request as ready for review July 25, 2023 16:07

thufschmitt force-pushed the spaces branch from bd4b584 to c0df677 Compare July 25, 2023 16:18

thufschmitt requested a review from fricklerhandwerk July 25, 2023 16:18

fricklerhandwerk requested changes Sep 5, 2023

View reviewed changes

src/nix/flake.md Outdated Show resolved Hide resolved

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved

src/nix/flake.md Outdated Show resolved Hide resolved

thufschmitt force-pushed the spaces branch from c0df677 to a79affd Compare September 5, 2023 11:31

thufschmitt requested a review from fricklerhandwerk September 5, 2023 11:33

thufschmitt force-pushed the spaces branch from 3508ce5 to ded93fc Compare September 5, 2023 12:10

fricklerhandwerk reviewed Sep 5, 2023

View reviewed changes

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved

src/nix/flake.md Outdated Show resolved Hide resolved

src/nix/flake.md Outdated Show resolved Hide resolved

src/nix/flake.md Outdated Show resolved Hide resolved

RasmusRendal and others added 4 commits September 22, 2023 10:06

Add a test for flake paths with spaces in them

d8cebae

Split the parseFlakeRefWithFragment function

e811374

Was starting to be very complex and hard to follow. Now the different cases should be easier to understand.

Document the percent-encoding mechanism

3411507

thufschmitt force-pushed the spaces branch from 256866a to 3411507 Compare September 22, 2023 08:08

fricklerhandwerk approved these changes Sep 22, 2023

View reviewed changes

src/nix/flake.md Outdated Show resolved Hide resolved

src/nix/flake.md Outdated Show resolved Hide resolved

Apply suggestions from code review

b343309

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

thufschmitt enabled auto-merge September 25, 2023 07:57

thufschmitt merged commit 9a78d87 into NixOS:master Sep 26, 2023
8 checks passed

thufschmitt mentioned this pull request Sep 28, 2023

Can't have unicode characters in URL #4563

Closed

fricklerhandwerk mentioned this pull request Oct 9, 2023

Improve Flakeref for Gitlab repos. #8773

Closed

8 tasks

This was referenced Oct 15, 2023

#6614 broke flake URL decoding for Gitlab #9161

Open

Fix and Improve Flakeref for Gitlab repos. #9163

Open

donovanglover mentioned this pull request Dec 20, 2023

nixos/nix-direnv: respect configured nix package NixOS/nixpkgs#275591

Merged

13 tasks

thufschmitt mentioned this pull request Jan 15, 2024

Nix develop not working with directory that contains special character on path #9751

Closed

thufschmitt mentioned this pull request Mar 1, 2024

Fix the parsing of the query of path-like flakerefs #10125

Open

Lagoja mentioned this pull request Mar 20, 2024

Devbox should handle spaces in paths jetify-com/devbox#1914

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for percent encoded filepaths for flakerefs #6614

Implement support for percent encoded filepaths for flakerefs #6614

RasmusRendal commented Jun 3, 2022 •

edited

thufschmitt commented Jun 14, 2022

RasmusRendal commented Jun 14, 2022 •

edited

thufschmitt commented Jun 20, 2022

crasm commented Jan 31, 2023

thufschmitt commented Feb 1, 2023

nomeata commented Feb 28, 2023

RasmusRendal commented Feb 28, 2023

thufschmitt commented Mar 4, 2023

thufschmitt Mar 4, 2023

nomeata Mar 4, 2023

thufschmitt commented Apr 20, 2023

fricklerhandwerk left a comment

thufschmitt commented May 9, 2023

fricklerhandwerk commented May 9, 2023

thufschmitt commented Jul 25, 2023

TheLostLambda commented Aug 28, 2023

thufschmitt commented Sep 5, 2023

fricklerhandwerk left a comment

thufschmitt commented Sep 22, 2023

fricklerhandwerk left a comment

crasm commented Sep 25, 2023

Implement support for percent encoded filepaths for flakerefs #6614

Implement support for percent encoded filepaths for flakerefs #6614

Conversation

RasmusRendal commented Jun 3, 2022 • edited

thufschmitt commented Jun 14, 2022

RasmusRendal commented Jun 14, 2022 • edited

thufschmitt commented Jun 20, 2022

crasm commented Jan 31, 2023

thufschmitt commented Feb 1, 2023

nomeata commented Feb 28, 2023

RasmusRendal commented Feb 28, 2023

thufschmitt commented Mar 4, 2023

thufschmitt Mar 4, 2023

Choose a reason for hiding this comment

nomeata Mar 4, 2023

Choose a reason for hiding this comment

thufschmitt commented Apr 20, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

thufschmitt commented May 9, 2023

fricklerhandwerk commented May 9, 2023

thufschmitt commented Jul 25, 2023

TheLostLambda commented Aug 28, 2023

thufschmitt commented Sep 5, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

thufschmitt commented Sep 22, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

crasm commented Sep 25, 2023

RasmusRendal commented Jun 3, 2022 •

edited

RasmusRendal commented Jun 14, 2022 •

edited