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

Support Nix depexts with opam env #5982

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

Conversation

RyanGibb
Copy link
Contributor

@RyanGibb RyanGibb commented May 30, 2024

Nix doesn't install packages in the traditional sense -- instead it puts them in a store and makes them available through environment variables. I.e., nix-shell -p gcc will drop us into a shell with gcc in the $PATH.

We can set appropriate environment variables with Opam to make system dependencies (depexts) available with Nix.
Similar to how nix-shell works under the hood, we create a Nix derivation such as

{ pkgs ? import <nixpkgs> {} }:
with pkgs;
let
  packages = [ gmp ];
  inputs = with buildPackages; packages ++ [ pkg-config ];
in
stdenv.mkDerivation {
  name = "opam-nix-env";
  nativeBuildInputs = inputs;

  phases = [ "buildPhase" ];

  buildPhase = ''
vars=("NIX_CC" "NIX_CC_FLAGS" "NIX_CFLAGS_COMPILE" "NIX_CC_WRAPPER_TARGET_HOST_x86_64_unknown_linux_gnu" "NIX_LDFLAGS" "PKG_CONFIG_PATH")
for var in "''${vars[@]}"; do
  escaped="$(echo "''${!var}" | sed -e 's/^$/@/' -e 's/ /\\ /g')"
  echo "$var    =       $escaped        Nix" >> $out
done
echo "PATH      =       $PATH   Nix"
  '';

  preferLocalBuild = true;
}

Which we can build to output a file with environment variables that make depexts available, in Opam's environment variable format. This file is a Nix store root, so it's dependencies won't be garbage collected by Nix until the file is removed.

This approach came from conversations with @dra27 and is distinct from previous approaches in that it supports providing development environment, which imperative installations with Nix don't #5332 (comment); and doesn't require the user to manage the environment outside of Opam, which would lead to a different workflow #5942.

Initial experiments were done using a package to set such environment variables https://github.com/RyanGibb/nix.opam.
However, in order to work with generic depexts (as opposed to just conf packages), to avoid cyclic dependencies (if conf packages depend on nix.opam but nix.opam depends on conf packages), and due to opam package sandboxing (nix derivation fetching would require opam sandboxing to be disabled or the nix store to be 'primed'), it's better implemented as a depext mechanism.

This has been tested and successfully creates an environment to provide packages conf-gmp and conf-libseccomp, as well as depexts directly.

@RyanGibb
Copy link
Contributor Author

Rebased on master

@RyanGibb RyanGibb force-pushed the nixos-depexts branch 3 times, most recently from 9e67415 to f348b9f Compare June 28, 2024 21:29
@RyanGibb
Copy link
Contributor Author

RyanGibb commented Jul 6, 2024

Looks like I'm running into NixOS/nix#10587 with the Docker image

phases = [ "buildPhase" ];

buildPhase = ''
vars=("NIX_CC" "NIX_CC_FLAGS" "NIX_CFLAGS_COMPILE" "NIX_CC_WRAPPER_TARGET_HOST_x86_64_unknown_linux_gnu" "NIX_LDFLAGS" "PKG_CONFIG_PATH")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to make these environment variables configurable in order to support new dependencies from Nix, which might require additional environment variables, without cutting a new release of Opam.

@RyanGibb
Copy link
Contributor Author

Rebased on master -- and could do with another depext CI run if it needs re-approval!

We support this by including `all_packages` in a switch in a call to
`OpamSolution.get_depexts`, including all system packages for the packages in a
switch as `required` in a call to `OpamSysInteract.install, and including all of
these in the Nix derivation generate by the Nix depext backend.
@RyanGibb
Copy link
Contributor Author

Okay! The CI is failing only due to ocaml/opam-repository#26261 (I've got it working locally with my branch of opam-repository)

master_changes.md Outdated Show resolved Hide resolved
src/client/opamSolution.ml Outdated Show resolved Hide resolved
src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
src/state/opamEnv.ml Outdated Show resolved Hide resolved
src/state/opamEnv.ml Outdated Show resolved Hide resolved
let map_sysmap f t =
let t = Option.get t in
Copy link
Member

Choose a reason for hiding this comment

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

could you explain why all these Option.get are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because I don't have access to the switch state here edd9030#diff-6426799f367266b2a65325c870931b51df35852e12ddbe8580e74f4eeeb58177R1606 afaik because it hasn't been created at this point. We need to pass it in to OpamSysInteract.install* in order for Nix to write to files in the switch, but we also need to support installing system packages before this switch has been created in the existing OpamClient logic (which will do nothing for the Nix backend).

OpamSysPkg.(Set.union nf sys.s_not_found)
| None -> avail, nf)
packages (OpamSysPkg.Set.empty, OpamSysPkg.Set.empty)
| None -> avail, req, nf)
Copy link
Member

Choose a reason for hiding this comment

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

i think it might be worth using a dedicated record instead of a tuple at this point. Otherwise it could become confusing very quickly with all those similar types around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have opamSysPkg.status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though, we're also passing around the couple of available and required a lot, which perhaps warrants a separate record.

RyanGibb and others added 3 commits July 18, 2024 18:06
Co-authored-by: Kate <kit-ty-kate@outlook.com>
also only attempt to read the nix environment file if the os-family = nixos
|} in
OpamFilename.write drvFile contents;
let envFile = OpamFilename.create dir (OpamFilename.basename (OpamFilename.raw "nix.env")) |> OpamFilename.to_string in
[`AsUser "nix-build", [ OpamFilename.to_string drvFile; "--out-link"; envFile ] ], None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be good if we could trigger a re-build of any packages that depend on depexts if the drv path changes, as if they link against a previous derivation it could be GC'd. An alternative might be to keep the old nix.env files around as Nix GC roots.

@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants