Skip to content

Give store paths a type in the Perl, and strip the store dir at the edges - #1872

Open
Ericson2314 wants to merge 1 commit into
masterfrom
perl-store-path
Open

Give store paths a type in the Perl, and strip the store dir at the edges#1872
Ericson2314 wants to merge 1 commit into
masterfrom
perl-store-path

Conversation

@Ericson2314

@Ericson2314 Ericson2314 commented Aug 24, 2026

Copy link
Copy Markdown
Member

Nix and the Rust parts of Hydra previously switched from using raw strings to a dedicated StorePath type. Now the Perl part makes the same switch.

This change does make the Perl code longer, but I think the clarity is well worth the verbosity.

Misc implementation notes:

  • A new Perl "type" (blessed ref): Nix::StorePath is a store path base name: a bare <hash>-<name>, no store directory, just like in Rust and C++.

  • The bindings speak it in both directions via an XS typemap, so no char * store paths remain, and no nix::parseStorePath/nix::printStorePath calls.

  • The database still holds full paths, so the conversion happens where a row is read or written, via Hydra::Component::InflateStorePath. (hydra-queue-runner also converts at the SQL boundary.) This is done via new Hydra::Schema::_storeDir state (with a getter/setter wrapper).

  • There are new printStorePath and parseStorePath functions in Perl, because we don't want observable behavior like the web UI and JSON API to change. Now, with StorePath sans store dir everywhere internally, we have to be careful to use those functions in all the boundary code.

  • The JSON API keeps emitting full paths without extra work: Hydra::Component::ToJSON reads columns with get_column, which is the raw database value and so bypasses inflation entirely. Noted there to prevent accidental breakage.

  • BuildProducts.path is not a store path but a path underneath one, so it inflates to a Hydra::RelativeStorePath pair with an accessor for each half. The column wants to be two columns, and a migration to make it two would not touch a caller.

  • Two DBIx::Class details to note:

    • deflation only runs on references, so StorePath being a blessed ref and not a string isn't merely cosmetic but load-bearing.

    • search/find conditions are never deflated, so a query against one of these columns needs to print by hand.

  • $MACHINE_LOCAL_STORE becomes a machineLocalStore() thunk, opened on first use rather than while Hydra::Helper::Nix is being "compiled". This is better in general because connecting to a daemon is such a non-trivial side effect. And it is better in particular because Hydra::Schema::storeDir reaches for Hydra::Helper::Nix itself, so eagerly opening would mean that merely asking where the store is opens one.

  • For the tests, with our non-default store directories, it is extra important that we do not prematurely open either the wrong store, or the right store before the directories it needs exist. So the harness uses Hydra::Schema::storeDir in setter mode to say which store the columns belong to, and loading the schema then has no side effects at all. (The test stores will then only be opened later, when they were before, and when they are actually needed.)

Some things this turned up on the way, none of them new:

  • common.tt stripped the store directory with substr(11), which would have eaten eleven characters of the hash the moment the value stopped being a full path.

  • NixManifest.pm and S3Backup.pm called computeFSClosure and queryPathInfo as free functions, which @EXPORT never provided; those paths could not have run.

  • Root.pm's log route rebuilt a full path from a URL segment with a hardcoded /nix/store. There is now no hardcoded store directory left in the Perl at all.

  • resolved.t derived a store directory with dirname because it had no better way to reach one; it now writes store paths and lets deflation supply it. content-addressed/basic.t asked in a comment for a way to query Nix for its store dir, and no longer needs one.

  • Hydra::Model::DB computes connect_info in a ->config call at the top level, so merely use-ing anything that reaches it pins the database before a test has set $HYDRA_DATABASE_URL. A test that wanted printStorePath and reached for Hydra::Helper::Nix to get it was enough to point the whole Catalyst app at the developer's own database. Nothing here fixes that; the tests just take care to load such modules at runtime, with the environment already in scope.

…dges

Nix and the Rust parts of Hydra previously switched from using raw
strings to a dedicated `StorePath` type. Now the Perl part makes the
same switch.

This change does make the Perl code longer, but I think the clarity is
well worth the verbosity.

Misc implementation notes:

- A new Perl "type" (blessed ref): `Nix::StorePath` is a store path base
  name: a bare `<hash>-<name>`, no store directory, just like in Rust
  and C++.

- The bindings speak it in both directions via an XS typemap, so no
  `char *` store paths remain, and no
  `nix::parseStorePath`/`nix::printStorePath` calls.

- The database still holds full paths, so the conversion happens where a
  row is read or written, via `Hydra::Component::InflateStorePath`.
  (`hydra-queue-runner` also converts at the SQL boundary.) This is done
  via new `Hydra::Schema::_storeDir` state (with a getter/setter
  wrapper).

- There are new `printStorePath` and `parseStorePath` functions in
  *Perl*, because we don't want observable behavior like the web UI and
  JSON API to change. Now, with `StorePath` sans store dir everywhere
  internally, we have to be careful to use those functions in all the
  boundary code.

- The JSON API keeps emitting full paths without extra work:
  `Hydra::Component::ToJSON` reads columns with `get_column`, which is
  the raw database value and so bypasses inflation entirely. Noted there
  to prevent accidental breakage.

- `BuildProducts.path` is not a store path but a path underneath one, so
  it inflates to a `Hydra::RelativeStorePath` pair with an accessor for
  each half. The column wants to be two columns, and a migration to
  make it two would not touch a caller.

- Two `DBIx::Class` details to note:

  - deflation only runs on references, so StorePath being a blessed ref
    and not a string isn't merely cosmetic but load-bearing.

  - `search`/`find` conditions are never deflated, so a query against
    one of these columns needs to print by hand.

- `$MACHINE_LOCAL_STORE` becomes a `machineLocalStore()` thunk, opened
  on first use rather than while `Hydra::Helper::Nix` is being
  "compiled". This is better in general because connecting to a daemon is
  such a non-trivial side effect. And it is better in particular because
  `Hydra::Schema::storeDir` reaches for `Hydra::Helper::Nix` itself, so
  eagerly opening would mean that merely asking where the store *is*
  opens one.

- For the tests, with our non-default store directories, it is extra
  important that we do not prematurely open either the wrong store, or
  the right store before the directories it needs exist. So the harness
  uses `Hydra::Schema::storeDir` in setter mode to say which store the
  columns belong to, and loading the schema then has no side effects at
  all. (The test stores will then only be opened later, when they were
  before, and when they are actually needed.)

Some things this turned up on the way, none of them new:

- `common.tt` stripped the store directory with `substr(11)`, which
  would have eaten eleven characters of the hash the moment the value
  stopped being a full path.

- `NixManifest.pm` and `S3Backup.pm` called `computeFSClosure` and
  `queryPathInfo` as free functions, which `@EXPORT` never provided;
  those paths could not have run.

- `Root.pm`'s `log` route rebuilt a full path from a URL segment with
  a hardcoded `/nix/store`. There is now no hardcoded store directory
  left in the Perl at all.

- `resolved.t` derived a store directory with `dirname` because it had
  no better way to reach one; it now writes store paths and lets
  deflation supply it. `content-addressed/basic.t` asked in a comment
  for a way to query Nix for its store dir, and no longer needs one.

- `Hydra::Model::DB` computes `connect_info` in a `->config` call at the
  top level, so merely `use`-ing anything that reaches it pins the
  database before a test has set `$HYDRA_DATABASE_URL`. A test that
  wanted `printStorePath` and reached for `Hydra::Helper::Nix` to get it
  was enough to point the whole Catalyst app at the developer's own
  database. Nothing here fixes that; the tests just take care to load
  such modules at runtime, with the environment already in scope.

Assisted-by: Claude Code (Opus 5)

@dasJ dasJ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From what I have seen (skimmed most of it, skipped the tests), I agree with the concept and what I have read looks good enough. After a rough sanity check, we might just be able to roll this without having to implement tests for every single place this touches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants