Make `initLibStore` a viable alternative by roberth · Pull Request #7732 · NixOS/nix

roberth · 2023-02-01T17:49:16Z

Motivation

Refactor initNix and initLibStore so that it's possible for a non-libmain process to initialize the nix libraries correctly.

Context

Close #7502

Implementation strategy:

Move calls between initialization functions in tiny commits for bisectability. I have tested at each intermediate step.
Expose some functions.

TODO:

decide https://github.com/NixOS/nix/pull/7732/files#r1093559808
reword some commits to make their context obvious
update perl bindings init

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

roberth · 2023-02-01T17:58:03Z

src/libutil/util.cc

+    if (!savedSignalMaskIsSet)
+        return;


@edolstra, would it make sense to define a default signal mask for child processes, or is a unix process like nix expected to inherit the signal mask of its caller and pass that mask on to its children (such as git, tar, ...)?

If you think it's sensible to unblock all signals in child processes, I would do this to make the library a bit more robust / work better out of the box.

Suggested change

if (!savedSignalMaskIsSet)

return;

if (!savedSignalMaskIsSet)

sigemptyset(&savedSignalMaskIsSet);

If you don't know or aren't confident whether it's acceptable unixing to unblock all signals in child processes, that's ok too; we'd just stick to the current, conservative approach.

Ericson2314 · 2023-02-01T22:11:01Z

Very nice to see this!

edolstra · 2023-02-02T09:33:02Z

src/libstore/globals.cc

+#if __APPLE__
+    if (hasPrefix(getEnv("TMPDIR").value_or("/tmp"), "/var/folders/"))
+        unsetenv("TMPDIR");
+#endif


Isn't this strictly worse than having it in initNix()? I don't think users of libstore would expect their environment to be messed with in this way.

This behavior exists for a reason, and for a libstore reason, so it's not strictly worse.

I've opened #7731 so this can be improved later. Until then I think we should stick to the current behavior, so that store users who should move from initNix to initLibStore don't regress because of this.

src/libutil/util.cc

src/libutil/hash.cc

src/libstore/globals.cc

roberth · 2023-02-03T17:15:02Z

src/libutil/util.cc

+void initLibUtil() {
+}


This is odd for now, but allows libutil to initialize a dependency if need be in the future.

nixos-discourse · 2023-02-13T15:53:59Z

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

https://discourse.nixos.org/t/2023-02-10-nix-team-meeting-minutes-31/25438/1

Ericson2314 · 2023-02-22T18:04:26Z

@edolstra Is anything more needed here? This looks good to me, especially with the follow-up issue @roberth created #7731

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. Using libstore without loading the config file is risky, as sqlite may then be misconfigured. See cachix/cachix#475

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. The goal of this reordering is to make initLibStore self-sufficient in a following commit.

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

It is required for the sandbox, which is a libstore responsibility; not just libmain. Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

This code is bad. We shouldn't unset variables in programs whose children may need them. Fixing one issue at a time, so postponing. See NixOS#7731 Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

Quote Why not initLibExpr()? initGC() is essentially that, but detectStackOverflow is not an instance of the init function concept, as it may have to be invoked more than once per process. Furthermore, renaming initGC to initLibExpr is more trouble than it's worth at this time.

libutil is a dependency of libstore, so it should always be initialized as such. libutil is also a dependency of libmain. Being explicit about this dependency might be good, but not worth the slight code complexity until the library structure gets more advanced. Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

src/libutil/util.hh

How signals should be handled depends on what kind of process Nix is integrated into. The signal handler thread used by the stand-alone Nix commands / processes may not work well in the context of other runtime systems, such as those of Python, Perl, or Haskell.

Versions older this are sufficiently old that we don't want to support them, and they require extra support code.

Left over from 9747ea8, NixOS#5821

Ericson2314 · 2023-04-12T02:17:23Z

Post release is a great time to merge this. And multiple things need it (Cachix I hear, RFC 134). @edolstra will be able to review this soon?

nixos-discourse · 2023-04-18T07:19:25Z

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

https://discourse.nixos.org/t/2023-04-17-nix-team-meeting-minutes-49/27379/1

roberth requested review from edolstra and thufschmitt as code owners February 1, 2023 17:49

roberth commented Feb 1, 2023

View reviewed changes

roberth changed the title ~~Make init lib store viable alternative~~ Make initLibStore a viable alternative Feb 1, 2023

Ericson2314 mentioned this pull request Feb 1, 2023

[RFC 0134] Carve out a store-only Nix NixOS/rfcs#134

Merged

infinisil mentioned this pull request Feb 1, 2023

Experimental python bindings #7735

Draft

14 tasks

edolstra reviewed Feb 2, 2023

View reviewed changes

src/libutil/util.cc Outdated Show resolved Hide resolved

edolstra reviewed Feb 2, 2023

View reviewed changes

src/libutil/hash.cc Outdated Show resolved Hide resolved

edolstra reviewed Feb 2, 2023

View reviewed changes

src/libstore/globals.cc Outdated Show resolved Hide resolved

roberth force-pushed the make-initLibStore-viable-alternative branch from a2a4b4f to 0a446da Compare February 3, 2023 17:10

roberth commented Feb 3, 2023

View reviewed changes

roberth force-pushed the make-initLibStore-viable-alternative branch 2 times, most recently from 61d7454 to 0fb7577 Compare February 5, 2023 13:43

thufschmitt assigned edolstra Feb 10, 2023

Ericson2314 approved these changes Feb 22, 2023

View reviewed changes

Ericson2314 added the RFC Related to an accepted RFC label Mar 15, 2023

Ericson2314 mentioned this pull request Mar 15, 2023

Tracking issue for RFC 134 #7868

Open

10 tasks

roberth added 8 commits April 7, 2023 16:24

Move OpenSSL init to initLibUtil

6e0b710

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

Move loadConfFile() to initLibStore

a692c43

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. Using libstore without loading the config file is risky, as sqlite may then be misconfigured. See cachix/cachix#475

Move initLibStore() immediately after initLibUtil()

9693076

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries. The goal of this reordering is to make initLibStore self-sufficient in a following commit.

Move sodium_init() to initLibStore()

a58be39

Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

Move preloadNSS() from initNix to initLibStore

e706ffa

It is required for the sandbox, which is a libstore responsibility; not just libmain. Part of an effort to make it easier to initialize the right things, by moving code into the appropriate libraries.

roberth force-pushed the make-initLibStore-viable-alternative branch from 0fb7577 to 074e00d Compare April 7, 2023 14:40

Ericson2314 reviewed Apr 7, 2023

View reviewed changes

src/libutil/util.hh Outdated Show resolved Hide resolved

roberth added 4 commits April 7, 2023 17:50

Require openssl >= 1.1.1

2445afd

Versions older this are sufficiently old that we don't want to support them, and they require extra support code.

libstore: Remove lockCPU dead code

1c0b680

Left over from 9747ea8, NixOS#5821

libstore: Remove lockCPU dead code

ddebeb9

Left over from 9747ea8, NixOS#5821

roberth force-pushed the make-initLibStore-viable-alternative branch from 6e5864b to ddebeb9 Compare April 7, 2023 15:51

roberth requested a review from edolstra April 8, 2023 17:19

Ericson2314 merged commit 72ffa7f into NixOS:master Apr 17, 2023

		void initLibUtil() {
		}

Uh oh!

Comments

Conversation

roberth commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Checklist for maintainers

Uh oh!

roberth Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Feb 1, 2023

Uh oh!

edolstra Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roberth Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Feb 13, 2023

Uh oh!

Ericson2314 commented Feb 22, 2023

Uh oh!

Uh oh!

Ericson2314 commented Apr 12, 2023

Uh oh!

nixos-discourse commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

roberth commented Feb 1, 2023 •

edited

Loading

roberth Feb 3, 2023 •

edited

Loading