-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Make initLibStore
a viable alternative
#7732
Make initLibStore
a viable alternative
#7732
Conversation
if (!savedSignalMaskIsSet) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
if (!savedSignalMaskIsSet) | |
return; | |
if (!savedSignalMaskIsSet) | |
sigemptyset(&savedSignalMaskIsSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
initLibStore
a viable alternative
Very nice to see this! |
#if __APPLE__ | ||
if (hasPrefix(getEnv("TMPDIR").value_or("/tmp"), "/var/folders/")) | ||
unsetenv("TMPDIR"); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a2a4b4f
to
0a446da
Compare
void initLibUtil() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd for now, but allows libutil to initialize a dependency if need be in the future.
61d7454
to
0fb7577
Compare
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 |
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.
0fb7577
to
074e00d
Compare
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
Left over from 9747ea8, NixOS#5821
6e5864b
to
ddebeb9
Compare
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? |
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 |
Motivation
Refactor
initNix
andinitLibStore
so that it's possible for a non-libmain process to initialize the nix libraries correctly.Context
Close #7502
Implementation strategy:
TODO:
reword
some commits to make their context obviousChecklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*