Make more path settings `PathsSetting`s #498

New issue

Open

opened 2024-09-01 22:41:29 +00:00 by rbt · 3 comments

rbt commented

2024-09-01 22:41:29 +00:00

Owner

PathsSetting has some nice ergonomics for nix.conf settings that are paths, but only a couple settings use it (diff-hook, repl-overlays, root-dir, log-dir, etc.

However, plenty of settings that are paths don't use the PathsSetting class: sandbox-paths, sandbox-build-dir, build-dir, allowed-impure-host-prefixes, ca-file, ssh-key, remote-program, secret-key-file, local-nar-cache.

Let's make some more path setting types (e.g. for types that only take one path) and use them! This will get us improved ergonomics (like relative paths or tilde paths, for settings where that's appropriate).

See also: #499.

[`PathsSetting`](https://git.lix.systems/lix-project/lix/src/commit/02eb07cfd539c34c080cb1baf042e5e780c1fcc2/src/libutil/config.hh#L356-L380) has some nice ergonomics for `nix.conf` settings that are paths, but only a couple settings use it (`diff-hook`, `repl-overlays`, `root-dir`, `log-dir`, etc. However, plenty of settings that are paths _don't_ use the `PathsSetting` class: `sandbox-paths`, `sandbox-build-dir`, `build-dir`, `allowed-impure-host-prefixes`, `ca-file`, `ssh-key`, `remote-program`, `secret-key-file`, `local-nar-cache`. Let's make some more path setting types (e.g. for types that only take one path) and use them! This will get us improved ergonomics (like relative paths or tilde paths, for settings where that's appropriate). See also: #499.

rbt added the

bug

label 2024-09-01 22:41:29 +00:00

rbt added

E/easy

E/help wanted

and removed

bug

labels 2024-09-01 22:43:06 +00:00

lix-bot commented

2024-10-08 21:15:28 +00:00

Member

This issue was mentioned on Gerrit on the following CLs:

commit message in cl/2052 ("libstore: use PathsSetting for more settings that are Path")
commit message in cl/2051 ("libutil: implement PathsSetting<PathSet>")

This issue was mentioned on Gerrit on the following CLs: * commit message in [cl/2052](https://gerrit.lix.systems/c/lix/+/2052) ("libstore: use `PathsSetting` for more settings that are Path") * commit message in [cl/2051](https://gerrit.lix.systems/c/lix/+/2051) ("libutil: implement `PathsSetting<PathSet>`")

just1602 commented

2024-10-08 21:33:19 +00:00

Hi 👋

I open the two CL above, but I had a couple of question that will probably show up in the review anyways:

I didn't find any test for the PathsSetting implementations, if there's some, let me know and I'll add some for the PathsSetting<PathSet> implementation
I'm really new to the codebase, so if there's stuff that can be clean up after we switch to PathsSetting, I can do it in the CL or create a new one with needed follow up changes, just let me know.
last time I wrote c++ it was almost a decade ago, and it was c++ 1998, so I'm not super up-to-date with everything, so if there's stuff wrong or that I should do differently let me know.

And as everyone, you probably have other priorities in your life and in the lix project, so if your plate is full right now, there's no worries, I'll wait for when you get some time to review that.

Hi 👋 I open the two CL above, but I had a couple of question that will probably show up in the review anyways: * I didn't find any test for the `PathsSetting` implementations, if there's some, let me know and I'll add some for the `PathsSetting<PathSet>` implementation * I'm really new to the codebase, so if there's stuff that can be clean up after we switch to `PathsSetting`, I can do it in the CL or create a new one with needed follow up changes, just let me know. * last time I wrote c++ it was almost a decade ago, and it was c++ 1998, so I'm not super up-to-date with everything, so if there's stuff wrong or that I should do differently let me know. And as everyone, you probably have other priorities in your life and in the lix project, so if your plate is full right now, there's no worries, I'll wait for when you get some time to review that.

❤️ 1

lix-project referenced this issue from a commit

2024-10-28 14:23:33 +00:00

libutil: implement `PathsSetting<PathSet>`

lix-project closed this issue

2024-10-28 14:23:34 +00:00

just1602 commented

2024-10-28 15:03:35 +00:00

Meh, it was a mistake on my side, in my commit message I use Fix, but it should have been Partialy fix or contribute to. If you can reopen it @rbt that would be great.

Meh, it was a mistake on my side, in my commit message I use `Fix`, but it should have been `Partialy fix` or `contribute to`. If you can reopen it @rbt that would be great.