Skip to content

Conversation

@edolstra
Copy link
Member

@edolstra edolstra commented May 4, 2025

Motivation

This makes the builtin builders (builtin:fetchurl etc) register themselves at runtime, so that DerivationBuilderImpl::runChild() doesn't need to know about them.

Context

I want to add a builtin:fetch-tree builtin builder (DeterminateSystems#49). However, this would require libstore to depend on libfetchers, which would be a cyclic dependency. So we need to be able to define builtin builders outside of libstore.


Add 👍 to pull requests you find important.

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

@edolstra edolstra requested a review from Ericson2314 as a code owner May 4, 2025 20:27
RegisterBuiltinBuilder(const std::string & name, BuiltinBuilder && fun)
{
if (!builtinBuilders) builtinBuilders = new BuiltinBuilders;
builtinBuilders->insert_or_assign(name, std::move(fun));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should registering multiple instances with the same name be ok and overwrite the value? Wouldn't that allow plugins to override standard builders?

@github-project-automation github-project-automation bot moved this to To triage in Nix team May 5, 2025
@Mic92 Mic92 added this to Nix team May 5, 2025
@roberth
Copy link
Member

roberth commented May 5, 2025

I want to add a builtin:fetch-tree builtin builder (DeterminateSystems#49).

We should do #9259 as a prerequisite so that

  • derivations are equivalent regardless of how their content-addressed inputs are produced
  • buildTime = true is not needed

@Mic92
Copy link
Member

Mic92 commented May 12, 2025

I want to add a builtin:fetch-tree builtin builder (DeterminateSystems#49).

We should do #9259 as a prerequisite so that

* derivations are equivalent regardless of how their content-addressed inputs are produced

* `buildTime = true` is not needed

Is this blocking this PR in any way? I like that src/libstore/unix/build/derivation-builder.cc no has less knowledge over builtin builders. Also make potential plugins easier to build.

@edolstra
Copy link
Member Author

@Mic92 No, that would be discussion for a future PR. This PR is just refactoring to provide the necessary infrastructure for registering builtin builders.

@Mic92 Mic92 merged commit 0f985fe into master May 13, 2025
25 checks passed
@Mic92 Mic92 deleted the register-builtin-builders branch May 13, 2025 06:50
@github-project-automation github-project-automation bot moved this from To triage to Done in Nix team May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants