Skip to content

Bindings bananza#12

Closed
Mic92 wants to merge 2140 commits intomasterfrom
bindings-bananza
Closed

Bindings bananza#12
Mic92 wants to merge 2140 commits intomasterfrom
bindings-bananza

Conversation

@Mic92
Copy link
Copy Markdown
Owner

@Mic92 Mic92 commented Sep 17, 2025

Motivation

Context


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Mic92 and others added 30 commits August 23, 2025 08:52
maintainers: Add script for release notes todo list
Limit to lenient parsing of non-standard URLs only where needed
Make more URLs parsed, most notably `FileTransferRequest::url`
…Time stat and implement windows version

Update src/libutil/windows/current-process.cc

Prefer `nullptr` over `NULL`

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>

Update src/libutil/unix/current-process.cc

Prefer C++ type casts

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>

Update src/libutil/windows/current-process.cc

Prefer C++ type casts

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>

Update src/libutil/unix/current-process.cc

Don't allocate exception

Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
This is extracted from the work in NixOS#13752
libexpr: Fix weird formatting after treewide reformat
I didn't want to block that PR on further code review while I figured
out these new (to us) C++23 goodies.
This is a workaround for NixOS#13515
(opening the SQLite DB randomly taking a couple of seconds on ZFS).

(cherry picked from commit a7fceb5)
…3ToHttpsURL

`ParsedS3URL::toHttpsUrl` Slight optimize
SQLite: fsync db.sqlite-shm before opening the database
It is suppposed to be "post build" not "during the build" after all. Its
location now matches that for the hook case (see elsewhere in
`DerivationdBuildingGoal`).

It was in a try-catch before, and now it isn't, but I believe that it is
impossible for it to throw `BuildError`, which is sufficient for this
code motion to be correct.
Handle empty ports with new URL parsing
I need this for some `ParseURL` improvements, but I figure this is
better to send as its own PR.

I changed the tests willy-nilly to sometimes use
`std::list<std::string_view>` instead of `Strings` (which is
`std::list<std::string>`).

Co-Authored-By: Sergei Zimmerman <sergei@zimmerman.foo>
• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/cd32a774ac52caaa03bcfc9e7591ac8c18617ced?narHash=sha256-VtMQg02B3kt1oejwwrGn50U9Xbjgzfbb5TV5Wtx8dKI%3D' (2025-08-17)
  → 'github:NixOS/nixpkgs/d98ce345cdab58477ca61855540999c86577d19d?narHash=sha256-O2CIn7HjZwEGqBrwu9EU76zlmA5dbmna7jL1XUmAId8%3D' (2025-08-26)

This update contains d1266642a8722f2a05e311fa151c1413d2b9653c, which
is necessary for the TOML timestamps to get tested via nixpkgsLibTests job.
This allows us to replace some very hacky and not correct string
concatentation in `HttpBinaryCacheStore`. It will especially be useful
with NixOS#13752, when today's hacks started to cause problems in practice,
not just theory.

Also make `fixGitURL` returned a `ParsedURL`.
Implement `parseURLRelative`, use in `HttpBinaryCacheStore`
With the migration to /nix/var/nix/builds we now have failing builds
when the derivation name is too long.
This change removes the derivation name from the temporary build to have
a predictable prefix length:

Also see: NixOS/infra#764
for context.
I am not sure how/why this started working. C++23?
…r-params-aggregate-initialize

No more `DerivationBuilderParams:` constructor!
xokdvium and others added 27 commits September 14, 2025 22:52
As evident from the number of tests that were holding this API completely
wrong (the end() iterator returned from find() is NEVER nullptr) we should
not have this footgun. A proper strong type guarantees that this confusion
will not happen again.

Also this will be helpful down the road when Bindings becomes something
smarter than an array of Attr.
A follow-up optimization will make it impossible to make a find function
that returns an iterator in an efficient manner. All consumer code can
easily use the `get` variant.
libexpr: Make Bindings::iterator a proper strong type instead of pointer
…3960, NixOS#13979)

remove 'pre' version suffix for non-releases (chokes Darwin ld)
- Use `const K`, not `K`, otherwise we don't get auto referencing of
  rvalues.

- Generalized the deleted overloads, because we don't care what the key
  type is --- we want to get rid of anything that has an rvalue map
  type.
Ensure that files are parsed/evaluated only once
meson: add soversion with nix version to give SONAME to libs
Since `nix flake check` doesn't produce a `result` symlink, it doesn't
actually need to build/substitute derivations that are already known
to have succeeded, i.e. that are substitutable.

This can speed up CI jobs in cases where the derivations have already
been built by other jobs. For instance, a command like

  nix flake check github:NixOS/hydra/aa62c7f7db31753f0cde690f8654dd1907fc0ce2

should no longer build anything because the outputs are already in
cache.nixos.org.

Based-on: DeterminateSystems#134
Based-on: https://gerrit.lix.systems/c/lix/+/3841
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
nix flake check: Skip substitutable derivations
This is a follow-on to NixOS#13995 which added soversion to the libraries
meson: refactor nix_soversion into nix-meson-build-support/common
.strip() removes individual chars whereas .replace() affects whole substring

Thanks @keszybz
Also test the APIs we just added.
nix-meson-build-support/common nix_soversion: fixup removal of 'pre'
$ curl -I https://www.gnu.org/licenses/old-licenses/lgpl-2.1.txt
:
Last-Modified: Wed, 18 Sep 2024 14:34:04 GMT
ETag: "6733-62265b29fd1ee"
:
COPYING: update to latest lgpl-2.1.txt
This will useful for unit tests.
Revert "tests/nixos: Fix daemon store reference in authorization test"
Add `read-only` setting to `dummy://` store for back compat.

Test by changing an existing test to use this instead, fixing a TODO.

Co-Authored-By: HaeNoe <git@haenoe.party>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Use `MemorySourceAccessor` in `DummyStore` so writes work
This changes the implementation of Bindings to allow
for a more space-efficient implementation of attribute
set merges. This is accomplished by "layering" over the "base" Bindings.
The top "layer" is naturally the right-hand-side of the update operator //.

Such an implementation leads to significantly better memory usage on
something like nixpkgs:

nix-env --query --available --out-path --file ../nixpkgs --eval-system x86_64-linux > /dev/null

Comparison against 2b0fd88 for x86_64-linux on nixpkgs f06c7c3b6f5074dbffcf02542fb86af3a5526afa:

| metric                 | mean_before     | mean_after      | mean_diff       | mean_%_change | p_value | t_stat  |
| -                      | -               | -               | -               | -             | -       | -       |
| cpuTime                | 21.1520         | 21.3414         | 0.1894          | 0.7784        | 0.3190  | 1.0219  |
| envs.bytes             | 461451951.6190  | 461451951.6190  | -               | -             | -       | -       |
| envs.elements          | 34344544.8571   | 34344544.8571   | -               | -             | -       | -       |
| envs.number            | 23336949.0952   | 23336949.0952   | -               | -             | -       | -       |
| gc.cycles              | 7.5238          | 7.2857          | -0.2381         | -4.6825       | 0.0565  | -2.0244 |
| gc.heapSize            | 1777848124.9524 | 1252162023.6190 | -525686101.3333 | -29.9472      | 0.0000  | -8.7041 |
| gc.totalBytes          | 3102787383.6190 | 2498431578.6667 | -604355804.9524 | -19.7704      | 0.0000  | -9.3502 |
| list.bytes             | 59928225.9048   | 59928225.9048   | -               | -             | -       | -       |
| list.concats           | 1240028.2857    | 1240028.2857    | -               | -             | -       | -       |
| list.elements          | 7491028.2381    | 7491028.2381    | -               | -             | -       | -       |
| nrAvoided              | 28165342.2381   | 28165342.2381   | -               | -             | -       | -       |
| nrExprs                | 1577412.9524    | 1577412.9524    | -               | -             | -       | -       |
| nrFunctionCalls        | 20970743.4286   | 20970743.4286   | -               | -             | -       | -       |
| nrLookups              | 10867306.0952   | 10867306.0952   | -               | -             | -       | -       |
| nrOpUpdateValuesCopied | 61206062.0000   | 25748169.5238   | -35457892.4762  | -58.8145      | 0.0000  | -8.9189 |
| nrOpUpdates            | 2167097.4286    | 2167097.4286    | -               | -             | -       | -       |
| nrPrimOpCalls          | 12337423.4286   | 12337423.4286   | -               | -             | -       | -       |
| nrThunks               | 29361806.7619   | 29361806.7619   | -               | -             | -       | -       |
| sets.bytes             | 1393822818.6667 | 897587655.2381  | -496235163.4286 | -36.7168      | 0.0000  | -9.1115 |
| sets.elements          | 84504465.3333   | 48270845.9524   | -36233619.3810  | -43.8698      | 0.0000  | -8.9181 |
| sets.number            | 5218921.6667    | 5218921.6667    | -               | -             | -       | -       |
| sizes.Attr             | 16.0000         | 16.0000         | -               | -             | -       | -       |
| sizes.Bindings         | 8.0000          | 24.0000         | 16.0000         | 200.0000      | -       | inf     |
| sizes.Env              | 8.0000          | 8.0000          | -               | -             | -       | -       |
| sizes.Value            | 16.0000         | 16.0000         | -               | -             | -       | -       |
| symbols.bytes          | 1368494.0952    | 1368494.0952    | -               | -             | -       | -       |
| symbols.number         | 109147.1905     | 109147.1905     | -               | -             | -       | -       |
| time.cpu               | 21.1520         | 21.3414         | 0.1894          | 0.7784        | 0.3190  | 1.0219  |
| time.gc                | 1.6011          | 0.8508          | -0.7503         | -37.1507      | 0.0017  | -3.6328 |
| time.gcFraction        | 0.0849          | 0.0399          | -0.0450         | -37.4504      | 0.0035  | -3.3116 |
| values.bytes           | 615968144.7619  | 615968144.7619  | -               | -             | -       | -       |
| values.number          | 38498009.0476   | 38498009.0476   | -               | -             | -       | -       |

Overall this does slow down the evaluator slightly (no more than ~10% in most cases),
but this seems like a very decent tradeoff for shaving off 33% of memory usage.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Sep 17, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bindings-bananza

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Mic92 Mic92 closed this Sep 17, 2025
@Mic92 Mic92 deleted the bindings-bananza branch September 17, 2025 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.