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

Add sandboxed building for FreeBSD using jails #9968

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

rhelmot
Copy link

@rhelmot rhelmot commented Feb 8, 2024

Motivation

Build isolation is good! In Linux, this is accomplished with namespaces (containers). The equivalent technology on FreeBSD is jails.

Context

This is part of my ongoing project to make FreeBSD a first class citizen in the nix world.

This was a fairly simple patch, just needed to add parallel implementations for all the sandboxed Linux build pieces.

The most fragile part of this implementation is the fact that there is a lot of global state that gets set up in order to construct the jail - the chroot dir in the nix store, the nullfs mounts (the FreeBSD equivalent of a bind mount), and the jail ID itself. Lots of steps have been taken to make sure these all get cleaned up, both at the end of the build and at the start of any rebuilds. It seems to be resilient to interruption.

This has been live-fire tested with my fork of nixpkgs for FreeBSD. It is able to build the stdenv without issue.

Please squash-merge this PR! It includes some changes that were later reverted, which don’t belong in this repository but instead in the FreeBSD ports repository.

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
Copy link
Member

This is very cool! But before supporting any new sandboxing methods, I would ideally like to de-spagetti the code that we already have. Are you up for helping out with that too?

@rhelmot
Copy link
Author

rhelmot commented Feb 8, 2024

Yeah, that sounds good. What are your design parameters?

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 8, 2024

I am not entirely sure, but to start, see in #8901 how I have some platform specific .cc and .hh files.

More broadly it's a process of untangling the spaghetti where we cannot tell what exactly we're gonna get until we do it :)

@edolstra
Copy link
Member

edolstra commented Feb 8, 2024

Ideally we would have subclasses of LocalDerivationGoal that provide the platform-specific code for building (such as sandboxing). Or create some abstract interface for build execution that LocalDerivationGoal can use.

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 8, 2024

Or create some abstract interface for build execution that LocalDerivationGoal can use.

Yes I would prefer that. I made LocalDerivationGoal a DerivationGoal subclass because it was an easy first step to split things up, but I don't think deep inheritance hierarchies is how this stuff aught to work.

@rhelmot
Copy link
Author

rhelmot commented Feb 9, 2024

I gave it a shot. Unfortunately, due to my lack of deep understanding of the requisite linux and macos internals, I wasn't able to do a whole lot more than moving verbose chunks of ifdef'd code into platform-specific files. However, I think this makes it a lot more readable.

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Feb 9, 2024
@Ericson2314
Copy link
Member

Discussed in the Nix team meeting. Conclusions:

  • Assigned to @Ericson2314 to follow-up
  • Can land the cleanups in a preperatory PR

details:

Contributor (@rhelmot) interested in adding a FreeBSD sandbox, willing to refactor the sandbox code to enable that
  • @edolstra: The refactoring is welcome, but we don't have the capacity to maintain the FreeBSD code

  • @rhelmot also revived the FreeBSD stdenv in Nixpkgs (Revive FreeBSD stdenv nixpkgs#254801)

  • @roberth: Would be great if we could make them code owner or similar

  • @edolstra: Mac bugs are painful because foreign to most maintainers and block features

    • @roberth: FreeBSD might be easier than MacOS for that (and also uncover the same bugs, making MacOS less of a trouble)
  • Proposal (@thufschmitt): If the refactorings allow having the FreeBSD sandbox non-intrusive, then we can accept it without commitments on maintaining it ourselves (like we already do for the broader FreeBSD support).

    • Agreement
  • @Ericson2314: In particular, we can merge the preperatory cleanups before we've decided on the FreeBSD-specific code.

Assigned to @Ericson2314 to follow-up

Extra note from me: hope jumping through these hoops is not too disheartening @rhelmot. You've done very good work here and in Nixpkgs on this stuff! I look forward to working with you on getting it all merged.

@rhelmot
Copy link
Author

rhelmot commented Feb 12, 2024

I'm fine with being made code owner, and I'm fine with rebasing my changes on top of a prep PR which carves out spaces for the sandbox to live. The categories I was able to pull out in terms of my attempted refactor were very much best-effort descriptions of what was already there.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-123/39775/1

@L-as
Copy link
Member

L-as commented Mar 7, 2024

@Ericson2314 @edolstra I've made some clean-ups locally for supporting WASM derivations in my local code. The clean-up involves removing LocalDerivationGoal and refactoring the building into a separate class that DerivationGoal uses. I currently have HookBuilder, NativeBuilder, and WasmBuilder (which isn't relevant to this PR but is what my goal is).
You could reasonably split out NativeBuilder into LinuxBuilder, BSDBuilder, etc.

EDIT:
This is the interface I made between DerivationGoal and the ways of building:
https://github.com/L-as/nix/blob/ce6d800bb513728b1ff92916ca5f64929eb698fb/src/libstore/build/builder-interface.hh (wasm-derivations branch)

@Ericson2314
Copy link
Member

@rhelmot I (sleepily) talked to @L-as a bit about this at Nix Con NA, but i am hoping that you and him and I and work together on these cleanups, and between the 3 of us it will be less onerous :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

6 participants