Skip to content

Move platform-specific code out of DerivationBuilder#13276

Merged
Ericson2314 merged 11 commits intomasterfrom
split-derivation-builder
May 27, 2025
Merged

Move platform-specific code out of DerivationBuilder#13276
Ericson2314 merged 11 commits intomasterfrom
split-derivation-builder

Conversation

@edolstra
Copy link
Member

@edolstra edolstra commented May 27, 2025

Motivation

This PR moves almost all of the Linux/Darwin-specific code out of DerivationBuilderImpl and into separate files/subclasses. I.e. we have the following class hierarchy:

DerivationBuilderImpl
|- LinuxDerivationBuilder
+- DarwinDerivationBuilder

DerivationBuilderImpl itself implements generic unsandboxed Unix building, but has virtual methods that allow the subclasses to provide sandboxing.

LinuxDerivationBuilder performs namespaced/chroot builds on Linux. It is not used on Linux if you have sandboxing disabled. (Currently the seccomp/personality code is part of this class, so we may want to split the class hierarchy a bit further.)

makeDerivationBuilder() instantiates the right class based on the OS type and whether sandboxing is enabled.

In the future, we should split the generic Unix code (e.g. user allocation, runChild(), pseudo-terminal allocation) from the platform-independent code (e.g. registerOutputs()), so we could have something like this:

DerivationBuilder
|- UnixDerivationBuilder
|   |- LinuxDerivationBuilder
|   +- DarwinDerivationBuilder
+- WindowsDerivationBuilder

Context


Add 👍 to pull requests you find important.

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

@edolstra edolstra marked this pull request as ready for review May 27, 2025 13:26
@edolstra edolstra requested a review from Ericson2314 as a code owner May 27, 2025 13:26
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Fantastic! Thanks for much for doing this @edolstra!

Comment on lines +2102 to +2104
// FIXME: do this properly
#include "linux-derivation-builder.cc"
#include "darwin-derivation-builder.cc"
Copy link
Member

Choose a reason for hiding this comment

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

I'll let this slide for now :)

Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what would "do this properly" look like? A separate file that has these includes and the function definition, or...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we'd have to move DerivationBuilderImpl into a header. But putting an impl class in a header kind of defeats the purpose. And there are probably some more refactorings that we want to do (like splitting DerivationBuilder and UnixDerivationBuilder) but we can do that in a future PR.

@Ericson2314 Ericson2314 merged commit 653a93a into master May 27, 2025
25 checks passed
@Ericson2314 Ericson2314 deleted the split-derivation-builder branch May 27, 2025 17:39
@roberth roberth added the backports rejected PR should not be backported (or at least not in full, or unless an overriding need arises) label Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backports rejected PR should not be backported (or at least not in full, or unless an overriding need arises)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants