NixOS / nix

Nix, the purely functional package manager
https://nixos.org/
GNU Lesser General Public License v2.1
11.45k stars 1.44k forks source link

Meson build for libexpr libflake, external C API, unit tests #10973

Open Ericson2314 opened 4 days ago

Ericson2314 commented 4 days ago

Motivation

Context

Draft because I am harmonizing some style things

Priorities and Process

Add :+1: to pull requests you find important.

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

Ericson2314 commented 3 days ago

I think https://github.com/NixOS/nix/issues/10909 is coming into play --- the unit tests want the internal headers, but I purposely didn't install them. @roberth, tell me what to do! :)

roberth commented 3 days ago

the unit tests want the internal headers, but I purposely didn't install them.

It's ok to make it not perfect, get meson done first, and improve later. That said, I think we could make a small step so you don't need to install the internal headers, which is to move those into libexpr. They're not useful without libexpr anyway. #include "nix_api_value.h" seems to be unused in nix_api_expr_internal.h.

So if you can move that header, I think we're set for now.


As for the big picture, a fine-grained dependency graph could look like this:

flowchart TB
    subgraph test
    libexpr-tests
    libexpr-c-tests-lib
    libexpr-c-tests-exe
    end

    libexpr --> libexpr-c-headers
    libexpr-c-tests-lib --> libexpr-c-headers
    libexpr-c-tests-exe --> libexpr-c-tests-lib
    libexpr-c-tests-exe --> libexpr-c
    libexpr-tests --> libexpr
    libexpr-c --> libexpr
    libexpr-c --> libexpr-c-headers

However, we can accept some simplifications and approximations until we get there.

Specifically, the libexpr --> libexpr-c-headers dep appears to consist of forward declarations only, so we don't need to represent that in our build graph. By removing that link, we can also collapse libexpr-c-headers into libexpr-c without creating the cycle libexpr --> libexpr-c --> libexpr.

flowchart TD
    subgraph test
    libexpr-tests
    libexpr-c-tests-lib
    libexpr-c-tests-exe
    end

    libexpr-c-tests-lib --> libexpr-c
    libexpr-c-tests-exe --> libexpr-c-tests-lib
    libexpr-c-tests-exe --> libexpr-c
    libexpr-tests --> libexpr
    libexpr-c --> libexpr

We can also accept rebuilding the libexpr-c-tests derivation(s) when libexpr-c or libexpr change.

flowchart TD
    subgraph test
    libexpr-tests
    libexpr-c-tests
    end

    libexpr-c-tests --> libexpr-c
    libexpr-tests --> libexpr
    libexpr-c --> libexpr

And finally we could merge libexpr-c-tests into libexpr-tests to arrive at pretty much the status quo:

flowchart TD
    subgraph test
    libexpr-tests
    end

    libexpr-tests --> libexpr-c
    libexpr-tests --> libexpr
    libexpr-c --> libexpr

Put these graphs in reverse and we have a plan. Oddly, I've made reducing build graph granularity look like progress. That's funny, because we'd lose the benefits like realisation performance and separation of concerns. Probably also non-derivation build performance, because it'd be easy to mess up and create unnecessary dependencies.

Ericson2314 commented 2 days ago

@roberth Thanks for the exhaustive breakdown. I just put the _internal.h header back -- I forgot you had already done that for nix-util-test

Looks like the remaining failure is the static build doesn't have static initializers that run.

Ericson2314 commented 2 days ago

OK this is finally ready!!

Ericson2314 commented 1 day ago

I think I addressed all review items? :) The CI issue is just the installer thing, again.

Ericson2314 commented 1 day ago

I'm fine doing #10995 first or something like it first, I left a comment there about the pre part, but that is the only blocker for me.

I can put the files back, but just to be on the same page, I think Git has no trouble at all with unmodified relocated files, so I don' think this move will be causing merge conflicts.

roberth commented 23 hours ago

I think Git has no trouble at all with unmodified relocated files

IIRC it depends on the git operation. Some things just work, others don't. I'm pretty sure I had to do some manual cherry pick-like work to do backports earlier.

roberth commented 23 hours ago

I've restored the ${version} behavior, or you could cherry-pick https://github.com/NixOS/nix/pull/10995/commits/e084316130a255313eb026db8bc64a653f5ffb0c here.

Ericson2314 commented 12 hours ago

OK the files are put back now