-
Notifications
You must be signed in to change notification settings - Fork 81
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
Rule to generate docker images with nix store paths #214
Conversation
This subpackage contains a default.nix that was used to build the package. The motivation is to then bundle several nixpkgs_package into a docker image
Now both types of repositories have: - A regular file called like the repository, acting as entry point. It is no longer a symlink, since it contained a user-dependent path - A ":srcs" filegroup with all sources In particular it is now possible to depend on @nixpkgs//:srcs. We want this in in order to run "nix-build" in a normal rule to generate a streamable docker image
@aherrmann Regarding #291 , does this look like a reasonable approach? Wondering why this was never reviewed / commented on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmadisetti Thank you for the ping! @jcpetruzza sorry for the super late reply, this fell of the plate.
Regarding #291 , does this look like a reasonable approach?
It's targetting a different issue, namely that of providing all needed build dependencies for remote execution. It could also be used for deployables, if the deployable you're looking for are Docker images, and if you don't mind potentially an over-approximation of your runtime dependencies.
@jcpetruzza regarding the remote execution aspect. How does this approach fare related to the incrementality concerns raised in #180. I.e. if one Nix package is changed and the image needs to be rebuilt. Doesn't this invalidate all build actions and effectively force a full rebuild of the project?
x = %{def}; | ||
y = if builtins.isFunction x then | ||
let | ||
formalArgs = builtins.functionArgs x; | ||
actualArgs = builtins.intersectAttrs formalArgs {inherit %{args};}; | ||
in | ||
x actualArgs | ||
else x; | ||
in | ||
y%{maybe_sel} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe more descriptive names help make this easier to understand?
x = %{def}; | |
y = if builtins.isFunction x then | |
let | |
formalArgs = builtins.functionArgs x; | |
actualArgs = builtins.intersectAttrs formalArgs {inherit %{args};}; | |
in | |
x actualArgs | |
else x; | |
in | |
y%{maybe_sel} | |
value_or_function = %{def}; | |
value = if builtins.isFunction value_or_function then | |
let | |
formalArgs = builtins.functionArgs value_or_function; | |
actualArgs = builtins.intersectAttrs formalArgs {inherit %{args};}; | |
in | |
value_or_function actualArgs | |
else value_or_function; | |
in | |
value%{maybe_sel} |
if arg_name == None: | ||
arg_name = opt | ||
else: | ||
arg_val = opt if arg_opt == "--arg" else "''%s''" % opt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this trigger quoting issues if a users passes in a quoted --argstr
option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, and now that I think about it, it may end up removing spaces in the user string. Should probably use "
instead, and manually escape "
, \
and ${
nix_file = cp(repository_ctx, repository_ctx.attr.nix_file) | ||
expr_args = [repository_ctx.path(nix_file)] | ||
src = repository_ctx.attr.nix_file | ||
cp(repository_ctx, src, label_to_path(src, prefix="nixpkg/src")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what this change does and why it's needed? IIUC this introduces an extra prefix and nests the copied files deeper. Wouldn't that break cross workspace location expansion references?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while, but I think the idea was that we want to have a nixpkgs/default.nix
on every package, but then we need to prevent this interfering with user provided files, so by putting the user input under nixpkgs/src
, we ensure there will be no conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hairiest merge res is around here. Didn't dig deep enough to figure out how to resolve- just took changes and hoped it worked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, rules_nixpkgs already creates a special sub-directory called bazel-support
, which houses the Nix output link used as GC-root.
So, user-code already has to avoid collision with that reserved name.
Would it be possible to generate the nixpkgs/default.nix
under that sub-directory instead? Then this change wouldn't need to introduce the nixpkgs/src
prefix.
Hi @aherrmann!
Good question; I guess it depends on whether bazel considers the container-image of a (host) platform definition part of the build input or not, but I don't really know what it does here. My setup follows this guide, so essentially I have something like this:
And to use RBE I then do something like:
So every time I bump/add a nix dependency, I push a new version of the image and update the value of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
nix_file = cp(repository_ctx, repository_ctx.attr.nix_file) | ||
expr_args = [repository_ctx.path(nix_file)] | ||
src = repository_ctx.attr.nix_file | ||
cp(repository_ctx, src, label_to_path(src, prefix="nixpkg/src")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hairiest merge res is around here. Didn't dig deep enough to figure out how to resolve- just took changes and hoped it worked
|
||
contents = [] | ||
for src in srcs: | ||
path_to_default_nix = repository_ctx.path(src.relative("default.nix")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default nix seems to resolve to the consuming library, is this correct? Maybe we should document that a default.nix is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
touch default.nix
seemed to let things run for me. But should be more general (e.g. flakes, or nothing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems to want all deps to have a default.nix
, even non-nix sources
Error in path: Not a regular file: /home/user/.cache/bazel/_bazel_user/8bbdcf70ee87482f767b06aac75f66d1/external/com_google_absl/absl/flags/default.nix
Could just check? I'll look into this a bit more
inherit name tag; | ||
|
||
contents = [ | ||
# Contents get copied to the top-level of the image, so we jus putt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*just put
AFAIK it does, see here. So, I would expect this to invalidate the entire build. That also matches what we've seen in experiments in the past.
Fair enough. How problematic this is depends on the use-case. Indeed, bumping the nixpkgs revision will likely invalidate most targets in a project, but is also not expected to happen very frequently. Adding or tweaking individual packages, e.g. Nix imports of utilities developed separately, or third-party dependencies, OTOH should ideally not have that problem. |
Closing this as it has been superseded by #351. Will try and get that PR merged instead. |
Context: I'm currently using bazel with remote execution using this branch; it's not an ideal solution, but it's a first step and there could be other people interested in trying it out.
The goal was to have a docker image that contains all the nix store paths that occur in the workspace, so that the remote executors also inherit them. For this, I'm introducing a repository rule
nixpkgs_docker_image
, that I'm then using in my workspace as follows:In
srcs
, one should include every usage of thenixpkgs_package
rule, either explicit (as@nixpkgs_valgrind
in this example) or implicit as those coming from the toolchain rules. One then gets two targets:@nix_deps_image//:image
: agenrule
that usesdockerTools.buildLayeredImage
to build a tarball@nix_deps_image//:stream
: ash_binary
that usesdockerTools.streamLayeredImage
to output the tarball on stdout.One then needs to ensure that the docker image is available before starting a remote build, either by manually pushing the image, or running a CI step.
All this relies on the
nixpkgs_package
rule creating anixpkg
subpackage in the package repository, containing the exact nix-expression used bynix-build
to create the package. Ideally, this would all be more transparent to the user, who would just mention the toolchain and the package insrcs
and the rule would figure out the rest, but for a proof-of-concept this is enough.The funny bit is in the
bazel
argument, what's this about? There is an unexpected issue coming from using bazel provisioned by nixpkgs (which I guess is the most common case forrules_nixpkgs
users): internal bazel files like@bazel_tools//tools/cpp:linux_cc_wrapper.sh.tpl
get the shebang mangled so that#!/usr/bin/env bash
, becomes, say#!/nix/store/5nr4gffsb5l72xpz6d5jbd0skih3r9i2-bash/bin/bash
and the problem is how to ensure that those paths ends up in the docker image as well. The workaround here is for the user to optionally provide anixpkgs_package
that should correspond to the exact same bazel being used on dev, CI, etc., so that whatever was used when mangling shebangs is available as a dependency.As far as I can see, this issue goes beyond
rules_nixpkgs
, in that a nixpkgs-provisioned bazel is hard to use with remote execution and may require some upstream fix. I think all the possible approaches listed in #180 would be affected by this.