Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build: use nix path-info to test for build output availability #1352

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jan 26, 2024

cc @dasJ @Ericson2314

Hacky workaround for NixOS/nix#9859, but after I managed to fix the issue on my end, I decided I could still submit it against upstream.


When using the Perl bindings, this usually takes the wrong store, i.e. auto even if store_uri is a flat-file binary cache.

See also NixOS/nix#9859

Using fork/exec here on purpose to make sure that the output of nix path-info isn't spammed into stderr. This could probably done with system("foo &>/dev/null"), but I didn't want to worry about shell injection issues then. And finally, I can't seem to silence STDERR for system with a list as argument. Not a Perl dev though, so not sure if there's a better way.

When using the Perl bindings, this usually takes the wrong store, i.e.
`auto` even if `store_uri` is a flat-file binary cache.

See also NixOS/nix#9859

Using fork/exec here on purpose to make sure that the output of `nix path-info`
isn't spammed into stderr. This could probably done with
`system("foo &>/dev/null")`, but I didn't want to worry about shell
injection issues then. And finally, I can't seem to silence STDERR for
`system` with a list as argument. Not a Perl dev though, so not sure if
there's a better way.
@Ericson2314
Copy link
Member

I don't quite understand the things you are saying in that issue, but I soon as well just add the missing functionality. We have a nix-next branch so we can try it out right away.

@lheckemann
Copy link
Member

Does this even work when using the nixos module? hydra-server.service doesn't seem to have nix on its PATH. I also wonder if this is an appropriate approach given the other ways in which the nix command could fail (experimental-features = nix-command missing?) and with no distinction drawn between that and the path not being valid.

@Ma27
Copy link
Member Author

Ma27 commented Jan 26, 2024

It definitely works since I just realized I forgot to silence stdout and now I get paths printed to hydra-server's journal whenever I open a build view. Not sure why yet 🙃

Nix is part of hydra-server's env:

hydra root ~ # cat /nix/store/d4w2gj8y1anymj9lsm3bih4v95f03bw3-hydra-0.1.20240126.1d1ec0b/bin/hydra-server|grep nix-
PATH=${PATH/':''/nix/store/7cibfivz81rpv8jhr53fj5hjlidpnykb-nix-2.19.3/bin'':'/':'}
PATH='/nix/store/7cibfivz81rpv8jhr53fj5hjlidpnykb-nix-2.19.3/bin'$PATH
export NIX_RELEASE='nix-2.19.3'

could fail (experimental-features = nix-command missing?)

Fair. Should probably use a nix-* equivalent then 🤷

@Ericson2314
Copy link
Member

I am looking at the perl binding stuff.

…well

`nix-store -q` (and `--outputs`) doesn't seem to complain if a path
doesn't exist in a store anymore, so let's do it that way.

This approach works fine on Nix 2.3 as well, it only warns about
`experimental-features` being an unknown option, but that's silenced
along with the rest of stderr.
@Ericson2314
Copy link
Member

I made NixOS/nix#9863 to generalize the Perl bindings so we can fix this the right way. It would be nice to replace this with PR against nix-next using that branch.

I think we can basically do a big sed to update Hydra: it's fine to call openStore() again and again (without an argument, that is) because there is caching.

Ma27 added a commit to Ma27/hydra that referenced this pull request Feb 7, 2024
Implements support for Nix's new Perl bindings[1]. The current state
basically does `openStore()`, but always uses `auto` and doesn't support
stores at other URIs.

Even though the stores are cached inside the Perl implementation, I
decided to instantiate those once in the Nix helper module. That way
store openings aren't cluttered across the entire codebase. Also, there
are two stores used later on - MACHINE_LOCAL_STORE for `auto`,
BINARY_CACHE_STORE for the one from `store_uri` in `hydra.conf` - and
using consistent names should make the intent clearer then.

This doesn't contain any behavioral changes, i.e. the build product
availability issue from NixOS#1352 isn't fixed. This patch only contains the
migration to the new API.

[1] NixOS/nix#9863
Ma27 added a commit to Ma27/hydra that referenced this pull request Feb 7, 2024
Replaces / Closes NixOS#1352

Consider the following setup:

* `store_uri` points to a "special" store, e.g. `s3://` or `file://`.
* Hydra runs inside e.g. an nspawn container and exclusively builds
  on remote systems.

Then, build products are never in the nspawn's `/nix/store`. This in
turn means that `$MACHINE_LOCAL_STORE->isValidPath` is always `false`
(or `0` 🤷) for output paths and thus Hydra wrongly claims that the
output got garbage collected.

Instead, use the BINARY_CACHE_STORE to look for the availability in the
correct store.

Everything touching the `drv` rather than the output paths still uses
MACHINE_LOCAL_STORE: this is because `hydra-eval-jobs` does `openStore()`
on `auto`, so the derivations end up there rather than on the
BINARY_CACHE_STORE.
Ma27 added a commit to Ma27/hydra that referenced this pull request Feb 12, 2024
Implements support for Nix's new Perl bindings[1]. The current state
basically does `openStore()`, but always uses `auto` and doesn't support
stores at other URIs.

Even though the stores are cached inside the Perl implementation, I
decided to instantiate those once in the Nix helper module. That way
store openings aren't cluttered across the entire codebase. Also, there
are two stores used later on - MACHINE_LOCAL_STORE for `auto`,
BINARY_CACHE_STORE for the one from `store_uri` in `hydra.conf` - and
using consistent names should make the intent clearer then.

This doesn't contain any behavioral changes, i.e. the build product
availability issue from NixOS#1352 isn't fixed. This patch only contains the
migration to the new API.

[1] NixOS/nix#9863
Ma27 added a commit to Ma27/hydra that referenced this pull request Feb 12, 2024
Replaces / Closes NixOS#1352

Consider the following setup:

* `store_uri` points to a "special" store, e.g. `s3://` or `file://`.
* Hydra runs inside e.g. an nspawn container and exclusively builds
  on remote systems.

Then, build products are never in the nspawn's `/nix/store`. This in
turn means that `$MACHINE_LOCAL_STORE->isValidPath` is always `false`
(or `0` 🤷) for output paths and thus Hydra wrongly claims that the
output got garbage collected.

Instead, use the BINARY_CACHE_STORE to look for the availability in the
correct store.

Everything touching the `drv` rather than the output paths still uses
MACHINE_LOCAL_STORE: this is because `hydra-eval-jobs` does `openStore()`
on `auto`, so the derivations end up there rather than on the
BINARY_CACHE_STORE.
Ma27 added a commit to Ma27/hydra that referenced this pull request Feb 12, 2024
Implements support for Nix's new Perl bindings[1]. The current state
basically does `openStore()`, but always uses `auto` and doesn't support
stores at other URIs.

Even though the stores are cached inside the Perl implementation, I
decided to instantiate those once in the Nix helper module. That way
store openings aren't cluttered across the entire codebase. Also, there
are two stores used later on - MACHINE_LOCAL_STORE for `auto`,
BINARY_CACHE_STORE for the one from `store_uri` in `hydra.conf` - and
using consistent names should make the intent clearer then.

This doesn't contain any behavioral changes, i.e. the build product
availability issue from NixOS#1352 isn't fixed. This patch only contains the
migration to the new API.

[1] NixOS/nix#9863
Ma27 added a commit to Ma27/hydra that referenced this pull request Feb 12, 2024
Replaces / Closes NixOS#1352

Consider the following setup:

* `store_uri` points to a "special" store, e.g. `s3://` or `file://`.
* Hydra runs inside e.g. an nspawn container and exclusively builds
  on remote systems.

Then, build products are never in the nspawn's `/nix/store`. This in
turn means that `$MACHINE_LOCAL_STORE->isValidPath` is always `false`
(or `0` 🤷) for output paths and thus Hydra wrongly claims that the
output got garbage collected.

Instead, use the BINARY_CACHE_STORE to look for the availability in the
correct store.

Everything touching the `drv` rather than the output paths still uses
MACHINE_LOCAL_STORE: this is because `hydra-eval-jobs` does `openStore()`
on `auto`, so the derivations end up there rather than on the
BINARY_CACHE_STORE.
Mic92 pushed a commit that referenced this pull request Aug 21, 2024
Implements support for Nix's new Perl bindings[1]. The current state
basically does `openStore()`, but always uses `auto` and doesn't support
stores at other URIs.

Even though the stores are cached inside the Perl implementation, I
decided to instantiate those once in the Nix helper module. That way
store openings aren't cluttered across the entire codebase. Also, there
are two stores used later on - MACHINE_LOCAL_STORE for `auto`,
BINARY_CACHE_STORE for the one from `store_uri` in `hydra.conf` - and
using consistent names should make the intent clearer then.

This doesn't contain any behavioral changes, i.e. the build product
availability issue from #1352 isn't fixed. This patch only contains the
migration to the new API.

[1] NixOS/nix#9863
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.

3 participants