Skip to content
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

Move plugins infra to libnixmain #11014

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Conversation

Ericson2314
Copy link
Member

Motivation

They are not actually part of the store layer, but instead part of the Nix executable infra (libraries don't need plugins, executables do).

Context

This is part of a larger project of moving all of our legacy settings infra to libmain, and having the underlying libraries just have plain configuration structs detached from any settings infra / UI layer.

Progress on #5638

Priorities and Process

Add 👍 to pull requests you find important.

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

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner July 2, 2024 19:20
@github-actions github-actions bot added store Issues and pull requests concerning the Nix store c api Nix as a C library with a stable interface labels Jul 2, 2024
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libraries don't need plugins, executables do

Executables using libraries also do, and libraries are only useful when executing them, which happens through executables, so this is a red herring.

Suppose I have a plugin that registers a builtin or a new hash function, and I configure that in nix.conf, then I would expect all of Nix to load it; not just the Nix CLI, but also other executables. Whether they call nix eval or -lnixexprc should not matter.

Coming to think of it, we could move plugins down into libutil so you can register your own hash functions, although the list of possible hashes is a store thing anyway, but also putting domain logic in libutil is wrong, so moving that is probably better. Plugins in libstore seems ok for now.

@Ericson2314
Copy link
Member Author

@roberth I did move them to a library libmain --- anyone that wants the old behavior can just link that library, and we can make C bindings for it too.

I want to move all the config stuff to libmain, actually. Anyone that wants that stuff can just use that library. But I really don't want this opinionated logic (plugin stuff or config stuff) in libexpr and libstore (or their dependencies) because it has no place being in a re-usable "backend" library when its a user-interface layer layer, and due to global variables + static initializers its not opt-in.

@roberth roberth self-requested a review July 3, 2024 16:15
@roberth
Copy link
Member

roberth commented Jul 3, 2024

Ok, I didn't see libmain as a viable library for outside consumption, but I see now that it does not depend on libexpr (like libcmd). Also not too big, fwiw.

It'd be nice to have some sort of description of each library.

Description: Nix Package Manager

Description: Nix Package Manager

Description: Nix Package Manager

🤔

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving it to a different library is already not so great for a "stable" interface, so let's at least not remove it.

Otherwise LGTM.

}
NIXC_CATCH_ERRS
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just remove it. Could you create a new c library for this?

@Ericson2314 Ericson2314 marked this pull request as draft July 7, 2024 13:15
In theory the warning is more noisy now, but in practice this will not
happen unless the client is older than 2.14 (highly unlikely).
They are not actually part of the store layer, but instead part of the
Nix executable infra (libraries don't need plugins, executables do).

This is part of a larger project of moving all of our legacy settings
infra to libmain, and having the underlying libraries just have plain
configuration structs detached from any settings infra / UI layer.

Progress on NixOS#5638
@Ericson2314
Copy link
Member Author

OK, the C function is restored

@Ericson2314 Ericson2314 marked this pull request as ready for review July 15, 2024 21:45
@roberth roberth merged commit b230c01 into NixOS:master Jul 17, 2024
11 checks passed
@Ericson2314 Ericson2314 deleted the plugins-libmain branch July 17, 2024 17:28
@Ericson2314
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c api Nix as a C library with a stable interface store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants