Commit graph

9823 commits

Author SHA1 Message Date
Pierre Bourdon 73c013a5df Merge "libexpr/gc-alloc: fix compilation with !HAVE_BOEHMGC" into main 2024-07-22 23:14:59 +00:00
Pierre Bourdon e76245f8e9
libexpr/gc-alloc: fix compilation with !HAVE_BOEHMGC
Fixes: 72ee25b402
Change-Id: Ib59386af1415a8ed4b53af24ec22a4ffa5e5877d
2024-07-23 00:50:09 +02:00
eldritch horrors 472ff1b833 libstore: keep Goal errors as unique_ptrs
Error is pretty large, and most goals do not fail. this alone more than
halves the size of Goal on x86_64-linux, from 720 bytes down to 344. in
derived classes the difference is not as dramatic, but even the largest
derived class (`LocalDerivationGoal`) loses almost 20% of its footprint

Change-Id: Ifda8f94c81b6566eeb3e52d55d9796ec40c7bce8
2024-07-22 19:01:40 +00:00
eldritch horrors 7bf1aff44a libstore: remove an always-defaulted argument
Change-Id: I3c7f17d5492a16bb54480fa1aa384b96fba72d61
2024-07-22 19:01:40 +00:00
eldritch horrors 58a91d70c9 libstore: use std::async instead of Goal threads
the goals are either already using std::async and merely forgot to
remove std::thread vestiges or they emulate async with threads and
promises. we can simply use async directly everywhere for clarity.

Change-Id: I3f05098310a25984f10fff1e68c573329002b500
2024-07-22 19:01:40 +00:00
eldritch horrors ad36fb43ad libstore: remove addToWeakGoals
under owner_less it's equivalent to insert(), only sometimes a little
bit faster because it does not construct a weak_ptr if the goal is in
the set already. this small difference in performance does not matter
here and c++23 will make insert transparent anyway, so we can drop it

Change-Id: I7cbd7d6e0daa95d67145ec58183162f6c4743b15
2024-07-22 19:01:40 +00:00
eldritch horrors d70e045f90 libstore: remove Goal::ecBusy
this should be an optional. "busy" is not an *exit* code!

Change-Id: Ic231cb27b022312b1a7a7b9602f32845b7a9c934
2024-07-22 19:01:40 +00:00
eldritch horrors 20f53346df libstore: remove unused Worker::waitForAnyGoal
Change-Id: Ia3ebd434b17052b6760ce74d8e20025a72148613
2024-07-22 19:01:40 +00:00
eldritch horrors c74eb81356 enable -Werror=suggest-override
*accidentally* overriding a function is almost guaranteed to be an
error. overriding a function without labeling it as such is merely
bad style, but bad style that makes the code harder to understand.

Change-Id: Ic0594f3d1604ab6b3c1a75cb5facc246effe45f0
2024-07-22 16:26:55 +00:00
eldritch horrors 0463cf2aef libexpr: fix -Wunused-const-variable warning
Change-Id: Ib986ece0ab2eff83e7abd7f1f915cd8f761827ad
2024-07-22 16:26:55 +00:00
alois31 2d4aca2546
libutil/logging: fix build without precompiled header
Commit 0109368c3f missed to include a required
header, which is not noticed when the precompiled header is enabled because
it's included in that. Also include it in the file so that the build without
precompiled header works too.

Change-Id: Id7a7979684b64f937f7f8191612952d73c113015
2024-07-21 12:54:31 +02:00
alois31 94a8e5fe0d Merge "libstore/binary-cache-store: use correct buffer size for NAR decompression" into main 2024-07-21 10:42:33 +00:00
jade 4fa6961aa2 Merge "gc: refactor the gc server thread out into a class without changing it" into main 2024-07-21 10:36:10 +00:00
alois31 391088900e
libstore/binary-cache-store: use correct buffer size for NAR decompression
Due to a leftover from a previous version where the buffer was allocated on the
stack, the change introduced in commit 4ec87742a1
accidentally passes the size of a pointer as the size of the buffer to the
decompressor. Since the former is much smaller (usually 8 bytes instead of 64
kilobytes), this is safe, but leads to considerable overhead; most notably, due
to excessive progress reports, which happen for each chunk. Pass the proper
buffer size instead.

Change-Id: If4bf472d33e21587acb5235a2d99e3cb10914633
2024-07-21 11:28:23 +02:00
Winter Cute 1917e6c765 Merge "Fix namespace warning being emitted if sandbox is disabled" into main 2024-07-20 22:14:33 +00:00
Qyriad 72ee25b402 libexpr: add a strongly typed version of gcAllocBytes()
This commit adds a new helper template function to gc-alloc.hh (which is
probably where you want to look at first, O great reviewer [custom file
ordering in review diffs when]), which uses a type argument to determine
the size to allocate, rather than making the caller use sizeof().

Change-Id: Ib5d138d91a28bdda304a80db24ea9fb08669ad22
2024-07-20 20:20:01 +00:00
Qyriad e67dac1d74 libexpr: rename confusing makeImmutableString -> gcCopyStringIfNeeded
The purpose of this function has little to do with immutability. Value's
strings are never mutated, and the point of this function is to
singleton empty strings.

Change-Id: Ifd41dd952409d54e4d3de9ab59064e6928b0e480
2024-07-20 20:20:01 +00:00
Qyriad a3361557e3 libexpr: refactor gc-agnostic helpers into one place
Change-Id: Icc4b367e4f670d47256f62a3a002cd248a5c2d3b
2024-07-20 20:20:01 +00:00
eldritch horrors 0109368c3f libutil: make basic loggers thread-safe
SimpleLogger is not fully thread-safe, and all loggers that wrap it are
also not safe accordingly. this does not affect much, but in rare cases
it can cause interleaving of messages on stderr when used with the json
or raw log formats. the fix applied here is a bit of a hack, but fixing
this properly requires rearchitecting the logger infrastructure. nested
loggers are not the most natural abstraction here, and it is biting us.

Change-Id: Ifbf34fe1e85c60e73b59faee50e7411c7b5e7c12
2024-07-20 12:33:49 +00:00
eldritch horrors d8c09b5836 libutil: remove warnOnce macro
it's only used once, and even that one use is highly questionable. more
instances of warnOnce should be much more principled than this has been

Change-Id: I5856570c99cb44462e700d753d0c706a5db03c4b
2024-07-20 12:33:49 +00:00
Winter Cute 3da41fdb82 Fix namespace warning being emitted if sandbox is disabled
If useChroot = false, and user namespaces aren't available for some
reason (e.g. within a Docker container), this fixes a pointless warning
being emitted, as we would never attempt to use them even if they were
available.

Change-Id: Ibcee91c088edd2cd19e70218d5a5802bff8f537b
2024-07-19 19:14:54 -04:00
jade 77ff799cc8 gc: refactor the gc server thread out into a class without changing it
This removes a *whole load* of variables from scope and enforces thread
boundaries with the type system.

There is not much change of significance in here, so the things to watch
out for while reviewing it are primarily that the destructor ordering
may have changed inadvertently, I think.

Change-Id: I3cd87e6d5a08dfcf368637407251db22a8906316
2024-07-19 20:55:55 +00:00
jade 22252825c4 Merge changes Id8b3d289,Ib75ab5b8,I3792eeb3 into main
* changes:
  Fixup a bunch of references to nixos.org manuals
  Add release notes for removing overflow from Nix language
  expr: fix a compiler warning about different signs in comparison
2024-07-19 18:52:46 +00:00
alois31 aba5f19680 Merge changes I829581a3,I0016970d,I5dac8e77,Ib7560fe5 into main
* changes:
  doc/release-notes: add for pretty printing improvements
  libexpr/print: do not show elided nested items when there are none
  libexpr/print: never show empty attrsets or derivations as «repeated»
  libexpr/print: pretty-print idempotently
2024-07-19 06:40:13 +00:00
jade 26e56780ca Fixup a bunch of references to nixos.org manuals
(plus one reference to CppNix github)

Change-Id: Id8b3d2897f3b54e286861805cfd421adc4d5de47
2024-07-18 19:27:33 +00:00
jade 50a63f8435 expr: fix a compiler warning about different signs in comparison
We know that variable is >=0, so we can just cast it to unsigned.

Change-Id: I3792eeb3ca43e6a507cc44c1a70584d42b2acd7b
2024-07-18 19:27:33 +00:00
jade 5ee1e6ea98 Merge changes Ib20e9aa0,I178a038b,I29c7de04 into main
* changes:
  docs: document the actual comparison rules instead of lies
  daemon: remove workaround for macOS kernel bug that seems fixed
  daemon: fix a crash bug "FATAL: exception not rethrown"
2024-07-18 17:40:32 +00:00
alois31 40c39aa5d2
libexpr/print: do not show elided nested items when there are none
When the configured maximum depth has been reached, attribute sets and lists
are printed with ellipsis to indicate the elision of nested items. Previously,
this happened even in case the structure being printed is empty, so that such
items do not in fact exist. This is confusing, so stop doing it.

Change-Id: I0016970dad3e42625e085dc896e6f476b21226c9
2024-07-18 18:41:34 +02:00
alois31 b5da823138
libexpr/print: never show empty attrsets or derivations as «repeated»
The repeated value detection logic exists so that the occurrence of large
common substructures does not fill up the screen or the computer's memory.
However, empty attribute sets and derivations (when their detection is enabled)
are always cheap to print, and in practice I have observed them to make up a
significant majority of the cases where I was annoyed by the repeated value
detection kicking in. Furthermore, `nix-instantiate --eval` already disables
this logic for empty attribute sets, and empty lists are already exempted
everywhere. For these reasons, always print empty attribute sets and
derivations as what they are.

Change-Id: I5dac8e7739f9d726b76fd0521ec46f38af94463f
2024-07-18 18:41:34 +02:00
alois31 81a0624d76
libexpr/print: pretty-print idempotently
When pretty-printing is enabled, previously an unforced thunk would trigger
indentation, even when it subsequently does not evaluate to a nested structure.
The resulting output looked inconsistent, and furthermore pretty-printing was
not idempotent (since pretty-printing the same value again, which is now fully
evaluated, will not trigger indentation).
When strict evaluation is enabled, force the item before inspecting its type,
so that it is properly known whether it contains a nested structure.
Furthermore, there is no need to cause indentation for unforced thunks, since
the very next operation will be printing them as `«thunk»`.

This is mostly a port of https://github.com/NixOS/nix/pull/11100 , but we only
force the item when it's going to be forced anyway due to strict
pretty-printing, and a new test was written since the REPL testing framework in
Lix is different.

Co-Authored-By: Robert Hensing <robert@roberthensing.nl>
Change-Id: Ib7560fe531d09e05ca6b2037a523fe21a26d9d58
2024-07-18 18:41:28 +02:00
eldritch horrors ef0de7c79f remove boost coroutine references
we no longer need these since sinkToSource and sourceToSink are gone.

Change-Id: Ibbf440e2cf71bf3e9f3b833af2d78a21fb1b3193
2024-07-16 01:50:16 +00:00
eldritch horrors dfedbc154f remove sourceToSink, sinkToSource, and boehm patch
Change-Id: I1379841299713175d0225b82a67f50660f9eb5e2
2024-07-16 01:50:16 +00:00
eldritch horrors d094dd0396 libstore: remove remaining sinkToSource uses
Change-Id: Id1ee0d2ad4a3774f4bbb960d76f0f76ac4f3eff9
2024-07-16 01:50:16 +00:00
eldritch horrors 6b4d46e9e0 libstore: remove WriteConn::sink fields
we no longer need these since we're no longer using sinks to serialize things.

Change-Id: Iffb1a3eab33c83f611c88fa4e8beaa8d5ffa079b
2024-07-16 00:57:42 +00:00
eldritch horrors a5d1f69841 libstore: generatorize protocol serializers
this is cursed. deeply and profoundly cursed. under NO CIRCUMSTANCES
must protocol serializer helpers be applied to temporaries! doing so
will inevitably cause dangling references and cause the entire thing
to crash. we need to do this even so to get rid of boost coroutines,
and likewise to encapsulate the serializers we suffer today at least
a little bit to allow a gradual migration to an actual IPC protocol.

(this isn't a problem that's unique to generators. c++ coroutines in
general cannot safely take references to arbitrary temporaries since
c++ does not have a lifetime system that can make this safe. -sigh-)

Change-Id: I2921ba451e04d86798752d140885d3c5cc08e146
2024-07-16 00:57:42 +00:00
eldritch horrors 5271424d14 libstore: remove a sinkToSouce from old daemon protocol
this doesn't have a test because this code path is only reached by
clients that predate 2.4, and we really should not be caring about
those any more right now. even the test suite doesn't, and the few
tests that might care are disabled because they will not even work

Change-Id: Id9eb190065138fedb2c7d90c328ff9eb9d97385b
2024-07-16 00:57:42 +00:00
eldritch horrors 4ec87742a1 libstore: rewrite the nar parser as a contents generator
this is not completely necessary at this point because the parser right
now already returns a generator to pass through all input data it read,
but the nar parser *was* very lax and would accept nars that weren't in
canonical form (defined as the form dumpPath would return). nar hashing
depends on these things, and as such rewriting the parser now allows us
to reject non-canonical nars that extract to the same store contents as
their canonical counterpart but have different nar hashes despite that.

Change-Id: Iccd319e3bd5912d8297014c84c495edc59019bb7
2024-07-16 00:57:42 +00:00
Qyriad c052716edd Merge changes I8d87c0e9,I25937702 into main
* changes:
  nix3-upgrade-nix: always use the /new/ nix-env to perform the installation
  libutil: implement a realPath() utility
2024-07-15 23:18:03 +00:00
eldritch horrors 3447dbfb2c libstore: rewrite narFromPath as generator
Change-Id: Ifa783c2c65c06ddd1d0212016d5bfd07666ea91c
2024-07-15 21:50:25 +00:00
Qyriad ae7eab49b9 nix3-upgrade-nix: always use the /new/ nix-env to perform the installation
Fixes #411.

Change-Id: I8d87c0e9295deea26ff33234e15ee33cc68ab303
2024-07-15 15:26:53 -06:00
Qyriad d9c51ec4e5 libutil: implement a realPath() utility
Just a wrapper around POSIX realpath().

Change-Id: I2593770285dbae573eace490efce5b272b00b001
2024-07-15 15:26:53 -06:00
nan-git 505640baec libexpr/eval.cc: remove unnecessary C string conversion
Change-Id: I5b7c21df84ff8ff64cf6a1e261fc3729a06bd4f6
2024-07-14 02:13:58 +00:00
jade 69e2ee5b25 daemon: remove workaround for macOS kernel bug that seems fixed
This was filed as https://github.com/nixos/nix/issues/7584, but as far
as I can tell, the previous solution of POLLHUP works just fine on macOS
14. I've also tested on an ancient machine with macOS 10.15.7, which
also has POLLHUP work correctly.

It's possible this might regress some older versions of macOS that have
a kernel bug, but I went looking through the history on the sources and
didn't find anything that looked terribly convincingly like a bug fix
between 2020 and today. If such a broken version exists, it seems pretty
reasonable to suggest simply updating the OS.

Change-Id: I178a038baa000f927ea2cbc4587d69d8ab786843
2024-07-13 01:17:14 +02:00
jade b3fb8d9822 daemon: fix a crash bug "FATAL: exception not rethrown"
This is caused by pthread_cancel effectively throwing a
not-specifically-identifiable C++ exception into the targeted thread,
which, if it is not rethrown, terminates the process entirely.

This is rather "impolite" behaviour, we would say. But thread
cancellation is *always* busted, and we should simply not use it where
unnecessary. It's particularly unnecessary when what we *actually* need
it for is, err, interrupting a poll(2).

That can in turn be achieved by simply listening to more stuff in the
poll, namely, a pipe, which we send a character to when needing to
stop the thread.

While looking at this code, we also investigated whether any of the
poll() madness is required, or was even *ever* required. Curiously we
found in the XNU kernel source code that the thing about needing to
listen to POLLHUP is probably *correct*, but switching it to POLLRDNORM
should not have made any difference at all. We've left a FIXME to look
into that further because what's written here is super janky.

94d3b45284/bsd/kern/sys_generic.c (L1751-L1758)

This is the crash on some Hydra machines:

Thread 1 (Thread 0x7f56b77776c0 (LWP 955542) (Exiting)):
0  0x00007f56b8e9b7dc in __pthread_kill_implementation () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
1  0x00007f56b8e49516 in raise () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
2  0x00007f56b8e31935 in abort () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
3  0x00007f56b8e327f3 in __libc_message_impl.cold () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
4  0x00007f56b8e8e8e9 in __libc_fatal () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
5  0x00007f56b8ea23c4 in unwind_cleanup () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
6  0x00007f56b9d2a1b8 in nix::triggerInterrupt() [clone .cold] () from /nix/store/sahgw550p621m9dy1pd7whl9c5g1g0p7-lix-2.90.0-rc1/lib/liblixutil.so
7  0x00007f56b990ac9d in std:🧵:_State_impl<std:🧵:_Invoker<std::tuple<nix::MonitorFdHup::MonitorFdHup(int)::{lambda()#1}> > >::_M_run() () from /nix/store/sahgw550p621m9dy1pd7whl9c5g1g0p7-lix-2.90.0-rc1/lib/liblixstore.so
8  0x00007f56b90e86d3 in execute_native_thread_routine () from /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libstdc++.so.6
9  0x00007f56b8e99a42 in start_thread () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6
10 0x00007f56b8f1905c in clone3 () from /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6

As for testing, we've started a daemon with this change and verified it
deals with HUPs correctly on x86_64-linux, but I don't think we can
easily test the destructor behaviour without whatever Hydra was
doing that broke.

Change-Id: I29c7de0425674494b6e43c075810126c3ff77363
2024-07-13 00:59:33 +02:00
jade 917c9bdee7 language: cleanly ban integer overflows
This also bans various sneaking of negative numbers from the language
into unsuspecting builtins as was exposed while auditing the
consequences of changing the Nix language integer type to a newtype.

It's unlikely that this change comprehensively ensures correctness when
passing integers out of the Nix language and we should probably add a
checked-narrowing function or something similar, but that's out of scope
for the immediate change.

During the development of this I found a few fun facts about the
language:
- You could overflow integers by converting from unsigned JSON values.
- You could overflow unsigned integers by converting negative numbers
  into them when going into Nix config, into fetchTree, and into flake
  inputs.

  The flake inputs and Nix config cannot actually be tested properly
  since they both ban thunks, however, we put in checks anyway because
  it's possible these could somehow be used to do such shenanigans some
  other way.

Note that Lix has banned Nix language integer overflows since the very
first public beta, but threw a SIGILL about them because we run with
-fsanitize=signed-overflow -fsanitize-undefined-trap-on-error in
production builds. Since the Nix language uses signed integers, overflow
was simply undefined behaviour, and since we defined that to trap, it
did.

Trapping on it was a bad UX, but we didn't even entirely notice
that we had done this at all until it was reported as a bug a couple of
months later (which is, to be fair, that flag working as intended), and
it's got enough production time that, aside from code that is IMHO buggy
(and which is, in any case, not in nixpkgs) such as
#445, we don't think
anyone doing anything reasonable actually depends on wrapping overflow.

Even for weird use cases such as doing funny bit crimes, it doesn't make
sense IMO to have wrapping behaviour, since two's complement arithmetic
overflow behaviour is so *aggressively* not what you want for *any* kind
of mathematics/algorithms. The Nix language exists for package
management, a domain where bit crimes are already only dubiously in
scope to begin with, and it makes a lot more sense for that domain for
the integers to never lose precision, either by throwing errors if they
would, or by being arbitrary-precision.

This change will be ported to CppNix as well, to maintain language
consistency.

Fixes: #423

Change-Id: I51f253840c4af2ea5422b8a420aa5fafbf8fae75
2024-07-13 00:59:33 +02:00
jade f9641b9efd libutil: add checked arithmetic tools
This is in preparation for adding checked arithmetic to the evaluator.

Change-Id: I6e115ce8f5411feda1706624977a4dcd5efd4d13
2024-07-13 00:56:37 +02:00
jade dde51af97d Use std::strong_ordering for version comparison
The actual motive here is the avoidance of integer overflow if we were
to make these use checked NixInts and retain the subtraction.

However, the actual *intent* of this code is a three-way comparison,
which can be done with operator<=>, so we should just do *that* instead.

Change-Id: I7f9a7da1f3176424b528af6d1b4f1591e4ab26bf
2024-07-12 16:48:28 +02:00
eldritch horrors 4b109ec1a8 libstore: remove upcast_goal
upcast_goal was only ever needed to break circular includes, but the
same solution that gave us upcast_goal also lets us fully remove it:
just upcast goals without a wrapper function, but only in .cc files.

Change-Id: I9c71654b2535121459ba7dcfd6c5da5606904032
2024-07-11 21:31:52 +00:00
eldritch horrors a5d431a911 libstore: turn copyNAR into a generator
Change-Id: Id452f6a03faa1037ff13af0f63e32883966ff40d
2024-07-11 20:37:27 +00:00
eldritch horrors 03db4efab9 libstore: turn the NAR parser into a passthrough generator
this will let us turn copyNAR into a generator as well, which in turn is
necessary to turn the users of copyNAR into generators without resorting
to sinkToSource coroutines. currently this uses the SerializingTransform
in all cases, even for copyNAR where it is not necessary. should this be
a performance problem we can easily swap out the transform for one which
does not produce any bytes of its own, but that should not be necessary.

Change-Id: I7e685879318fcbb78d8b88abfddd7752360eb0ce
2024-07-11 20:37:27 +00:00