Support arbitrary stores in Perl bindings #9863
Conversation
Ericson2314
force-pushed
the
perl-open-other-store
branch
2 times, most recently
from
cfd638f
to
e3ccb7e
Compare
|
I'll keep this a draft until we have a draft of the Hydra changes ready to go, CC @Ma27. |
|
|
||
| use Nix::Store; | ||
|
|
||
| my $s = new Nix::Store("dummy://"); |
There was a problem hiding this comment.
fwiw perlcritic complains about that and suggests to use Nix::Store->new() instead (noticed that while modifying Hydra for that).
There was a problem hiding this comment.
Fine with me! I don't know Perl enough to disagree :)
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
|
NixOS/hydra#1359 is here, so undrafting! |
Ericson2314
force-pushed
the
perl-open-other-store
branch
from
e3ccb7e
to
23b651e
Compare
There was a problem hiding this comment.
does newRV_noinc need to be available, got compiled away?
I'm getting:
$ perl -I ./result/lib/perl5/site_perl/5.36.0 -e 'use Nix::Store;'
Can't load './result/lib/perl5/site_perl/5.36.0/x86_64-linux-thread-multi/auto/Nix/Store/Store.so' for module Nix::Store: ./result/lib/perl5/site_perl/5.36.0/x86_64-linux-thread-multi/auto/Nix/Store/Store.so: undefined symbol: Perl_newRV_noinc at /nix/store/sjm6zhcjq1l6ghcal55vmpy48zd0rhm7-perl-5.38.2/lib/perl5/5.38.2/x86_64-linux-thread-multi/DynaLoader.pm line 206.
at -e line 1.
Compilation failed in require at -e line 1.
BEGIN failed--compilation aborted at -e line 1.
(or i'm using perl wrong, very likely)
Fix NixOS#9859 It's a breaking change but that's fine; we can just update Hydra to use the new bindings.
Ericson2314
force-pushed
the
perl-open-other-store
branch
from
23b651e
to
bc08502
Compare
|
Not really sure @tomberek, I don't know it well either. I updated the |
| if (!libStoreInitialized) { | ||
| initLibStore(); |
There was a problem hiding this comment.
initLibStore() internally already has a initLibStoreDone flag, so libStoreInitialized seems superfluous.
There was a problem hiding this comment.
That seems to be for a slightly different purpose? It doesn't make it run one, it just makes sure you ran it at least once.
(It could work the way this does, but I rather do that in a follow-up PR.)
| libStoreInitialized = true; | ||
| } | ||
| if (items == 1) { | ||
| _store = openStore(); |
There was a problem hiding this comment.
It's nicer to use the static initializer trick (because it's atomic), i.e.
static auto _store = {( openStore(); )};
RETVAL = new StoreWrapper {
.store = ref<Store>{_store}
};
There was a problem hiding this comment.
Wouldn't that mean the default store is always opened?
|
Because my two questions, I think it would be better to come back to those points in a follow-up PR. |
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
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
Motivation
Fix #9859
Context
It's a breaking change but that's fine; we can just update Hydra to use the new bindings.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
CC @Ma27