Create `StoreReference` and use it in `Machine` by Ericson2314 · Pull Request #9839 · NixOS/nix

Ericson2314 · 2024-01-23T20:46:15Z

Motivation

Separating parsing the URI from opening the store allows for a more robust Machine data type that is easier to use.

Context

Helps with Hydra dedup too NixOS/hydra#1164

~~Depends on #9850~~

Priorities and Process

Add 👍 to pull requests you find important.

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

cole-h · 2024-01-23T22:58:51Z

src/libstore/store-uri.cc

+}
+
+
+std::string StoreURI::render() const


Is there a reason you chose "render" over "to_string"? Seems like an odd choice to me.

src/libstore/store-uri.hh

src/libstore/store-api.hh

src/libstore/store-uri.hh

In particular `local://<path>` and `unix://` (without any path) now work, and mean the same things as `local` and `daemon`, respectively. We thus now have the opportunity to desguar `local` and `daemon` early. This will allow me to make a change to NixOS#9839 requested during review to desugar those earlier.

In particular `local://<path>` and `unix://` (without any path) now work, and mean the same things as `local` and `daemon`, respectively. We thus now have the opportunity to desguar `local` and `daemon` early. This will allow me to make a change to NixOS#9839 requested during review to desugar those earlier. Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>

nixos-discourse · 2024-02-26T08:02:28Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-03-nix-team-meeting-127/40309/1

In particular `local://<path>` and `unix://` (without any path) now work, and mean the same things as `local` and `daemon`, respectively. We thus now have the opportunity to desguar `local` and `daemon` early. This will allow me to make a change to NixOS#9839 requested during review to desugar those earlier. Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>

roberth · 2024-05-21T16:45:24Z

src/build-remote/build-remote.cc

 static AutoCloseFD openSlotLock(const Machine & m, uint64_t slot)
 {
-    return openLockFile(fmt("%s/%s-%d", currentLoad, escapeUri(m.storeUri), slot), true);
+    return openLockFile(fmt("%s/%s-%d", currentLoad, escapeUri(m.storeUri.render()), slot), true);


I think we have a problem here. Considering that we also have params in the URI, which do not affect the identity of the store to be opened, is this actually suitable for use in the name of a lock?
It's a pre-existing issue, fwiw.

Yes definitely a preexisting issue. If anything, we're better able to deal with it now.

src/libstore/ssh-store-config.hh

roberth · 2024-05-21T20:03:52Z

src/libstore/machines.cc

+    storeUri([&storeUri]{
+        try {
+            return StoreReference::parse(storeUri);
+        } catch (UsageError &) {


This is nasty. Should we deprecate this old syntax?

Could this be done during config parsing instead? That way nix show-config will show the correct syntax.

Also no chance to accidentally keep using this hack when we improve the settings format.

I would like to parse more eagerly, but that felt like a change best kept for later. (Similarly, using Setting<StoreReference> for a bunch of things.)

Right now, this is just supposed to match the back compat hack that was there before.

I just restored the old method of compat for now. We can decide separately whether such try-catch is worth it.

roberth · 2024-05-21T20:04:58Z

src/libstore/machines.cc

+        } catch (UsageError &) {
+            // Backwards compatibility: if the URI is invalid, it might
+            // be bare host name.
+            return StoreReference::parse("ssh://" + storeUri);


If this fails, we should throw the error for the unmodified string, not the new one.

I just restored the old method of compat for now. We can decide separately whether such try-catch is worth it.

src/libstore/store-reference.hh

src/libstore/machines.cc

For long expressions, one argument or parameter per line is just easier.

Need to decouple parsing from actually opening a store for Machine configs. Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com> Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

This will be needed for the next step. Also allows us to write round trip tests.

This makes the remote builder abstract syntax more robust.

With NixOS/nix#9839, the `storeUri` field is much better structured, so we can use it while still opening the SSH connection ourselves.

Ericson2314 requested review from edolstra and thufschmitt as code owners January 23, 2024 20:46

github-actions bot added the store Issues and pull requests concerning the Nix store label Jan 23, 2024

cole-h reviewed Jan 23, 2024

View reviewed changes

thufschmitt reviewed Jan 24, 2024

View reviewed changes

src/libstore/store-uri.hh Outdated Show resolved Hide resolved

src/libstore/store-api.hh Outdated Show resolved Hide resolved

roberth reviewed Jan 24, 2024

View reviewed changes

src/libstore/store-uri.hh Outdated Show resolved Hide resolved

src/libstore/store-uri.hh Outdated Show resolved Hide resolved

src/libstore/store-uri.hh Outdated Show resolved Hide resolved

Ericson2314 mentioned this pull request Jan 25, 2024

Ensure all store types support "real" URIs #9850

Merged

Ericson2314 marked this pull request as draft February 23, 2024 14:00

Ericson2314 force-pushed the more-machine-cleanup branch from 0998173 to 2ca7fce Compare May 21, 2024 16:09

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 21, 2024

Ericson2314 force-pushed the more-machine-cleanup branch from 2ca7fce to 94e43a2 Compare May 21, 2024 16:14

Ericson2314 marked this pull request as ready for review May 21, 2024 16:20

Ericson2314 force-pushed the more-machine-cleanup branch 2 times, most recently from 82f2d76 to 5660d44 Compare May 21, 2024 16:50

roberth reviewed May 21, 2024

View reviewed changes

Ericson2314 changed the base branch from master to nix-init-command May 21, 2024 17:11

Ericson2314 changed the base branch from nix-init-command to master May 21, 2024 17:11

Ericson2314 changed the title ~~Create StoreURI and use it in Machine~~ Create StoreReference and use it in Machine May 21, 2024

roberth reviewed May 21, 2024

View reviewed changes

Ericson2314 force-pushed the more-machine-cleanup branch 2 times, most recently from 849c8ab to a63c888 Compare May 22, 2024 12:49

Ericson2314 and others added 3 commits May 22, 2024 09:20

Slightly change formatting style

1d6c231

For long expressions, one argument or parameter per line is just easier.

Add StoreReference::render

b59a7a1

This will be needed for the next step. Also allows us to write round trip tests.

Use the new StoreReference in Machine

b3ebcc5

This makes the remote builder abstract syntax more robust.

Ericson2314 force-pushed the more-machine-cleanup branch from a63c888 to b3ebcc5 Compare May 22, 2024 13:22

roberth approved these changes May 22, 2024

View reviewed changes

Ericson2314 merged commit 4a19f4a into NixOS:master May 22, 2024

Ericson2314 deleted the more-machine-cleanup branch May 22, 2024 21:02

Ericson2314 mentioned this pull request May 23, 2024

Make store setting and store flags use StoreReference #10761

Closed

Ericson2314 added a commit to NixOS/hydra that referenced this pull request May 23, 2024

Utilize nix::Machine more fully

d55bea2

With NixOS/nix#9839, the `storeUri` field is much better structured, so we can use it while still opening the SSH connection ourselves.

Ericson2314 mentioned this pull request May 23, 2024

Utilize nix::Machine more fully NixOS/hydra#1385

Merged

Uh oh!

Conversation

Ericson2314 commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Priorities and Process

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixos-discourse commented Feb 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ericson2314 commented Jan 23, 2024 •

edited

Loading