Skip to content

perl: run initLibStore() on openStore()#7705

Merged
roberth merged 1 commit intoNixOS:masterfrom
Ma27:fix-initNix-in-perl-bindings
Feb 3, 2023
Merged

perl: run initLibStore() on openStore()#7705
roberth merged 1 commit intoNixOS:masterfrom
Ma27:fix-initNix-in-perl-bindings

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jan 28, 2023 •
edited by roberth
Loading

Motivation

Fix calling openStore() from Perl code (needed e.g. in Hydra)

Context

Since #7478 it's mandatory that initLibStore() is called for store operations. However that's not the case when running openStore() in Perl using the perl-bindings. That breaks e.g. hydra-eval-jobset when built against Nix 2.13 which uses small portions of the store API.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

This was referenced Jan 28, 2023
Closed
Merged
@cole-h cole-h mentioned this pull request Jan 28, 2023
Open
3 tasks
@edolstra
Copy link
Member

edolstra commented Jan 30, 2023

Shouldn't it call initLibStore() instead? initNix() is pretty invasive, e.g. it messes with signal handlers.

If you do need to use initNix(), then the call to loadConfFile() should be removed since initNix() already does that.

@roberth
Copy link
Member

roberth commented Feb 1, 2023

Shouldn't it call initLibStore() instead?

I'm making some changes to make that suggestion work.

@Ma27
Copy link
Member Author

Ma27 commented Feb 1, 2023

@roberth @edolstra so, how shall we proceed here? I guess for Hydra's use-case initLibStore() is sufficient, but I'm not so sure about other things using the Perl bindings.

@jkarni jkarni mentioned this pull request Feb 2, 2023
Closed
@Ericson2314
Copy link
Member

Ericson2314 commented Feb 2, 2023

I would just do initLibStore. If it worked before without any such init, then it will work today with a no-op initLibStore.

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 2, 2023

(Really I would consider all initialization for sake of ambient authority tech debt, it is indeed unclear when it is needed! As we improve the situation over time with explicit capabilities one passes around, it will become clearer what one is supposed to do. @roberth's PR is still good for at least narrowing the scope of which things need the ambient authority; it is like a triaging step.)

@Ma27
perl: run initLibStore() on openStore()
51013da 
Since NixOS#7478 it's mandatory that `initLibStore()` is called for store
operations. However that's not the case when running `openStore()` in
Perl using the perl-bindings. That breaks e.g. `hydra-eval-jobset` when
built against Nix 2.13 which uses small portions of the store API.
@Ma27 Ma27 force-pushed the fix-initNix-in-perl-bindings branch from b7d5047 to 51013da Compare February 2, 2023 15:00
Ericson2314
Ericson2314 approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Now just rename the PR title! :)

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 2, 2023 •
edited
Loading

This is future work, but I am thinking another lesson here is that bindings (Perl, but we wish to have more in the future) should have basic tests.

@Ma27 Ma27 changed the title perl: run initNix() on openStore() perl: run initLibStore() on openStore() Feb 2, 2023
@Ma27
Copy link
Member Author

Ma27 commented Feb 2, 2023

Now just rename the PR title! :)

Also updated the PR's description.

Also, I guess this is a candidate for backporting to 2.13?

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 2, 2023

Yes it should be, since it fixes a regression.

@Ma27
Copy link
Member Author

Ma27 commented Feb 3, 2023

Yes it should be, since it fixes a regression.

@Ericson2314 can you add the necessary label, would've done it, but I can't %)

TikhonJelvis added a commit to TikhonJelvis/dotfiles that referenced this pull request Feb 3, 2023
@TikhonJelvis
Add workaround for bug in nix-serve (or nix itself?)
c5860fc 
See [#7704][1] and [#7705][2] for details.

[1]: NixOS/nix#7704
[2]: NixOS/nix#7705
@roberth roberth merged commit 895dfc6 into NixOS:master Feb 3, 2023
@github-actions github-actions bot mentioned this pull request Feb 3, 2023
Merged
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Successfully created backport PR for 2.13-maintenance:

@roberth
Copy link
Member

roberth commented Feb 3, 2023

Merged so that we can potentially deliver the fix sooner.

A follow-up with a test would be much appreciated.

@Ma27 Ma27 deleted the fix-initNix-in-perl-bindings branch February 3, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

+1 more reviewer

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@roberth roberth

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Ma27 @edolstra @roberth @Ericson2314