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

Enable remote nix builds #239

Closed
wants to merge 1 commit into from
Closed

Enable remote nix builds #239

wants to merge 1 commit into from

Conversation

googleson78
Copy link
Contributor

@googleson78 googleson78 commented Jun 15, 2022

@layus said there are some new issues with this.

Also need to resolve all the TODOs and add tests.

Co-authored-by: Guillaume Maudoux <[email protected]>
@layus
Copy link
Collaborator

layus commented Jun 15, 2022

I would love feedback on this one. And maybe a test that uses nixbuild.net to ensure that it is at least usable.

As for the things that do not work, well

  1. it is hard to specify the ENV_NIX_REMOTE_KEY_FILE when it is located in the repository. You need to expand %workspace% and bazel will not do that for environment variables, so you need wrapper script AFAIK.
  2. The cache key needs special permissions for ssh to accept it, so that is another fixture that needs to happen in the wrapper.
  3. The remote server is suddenly accessible to everyone, making it very insecure.
    • We can maybe drop the ssh-ing part for now, and accept that the paths may get garbage collected.
  4. This is mostly about remote nix execution, with the remote used as a cache. It may be beneficial to also setup a remote cache, but I do not know how these play together. And the commands we need here heavily depend on the architecture of the remote builders and the cache.

I guess the question is: is is okay to have this as an experimental feature ? How could we mark it as such ?

Copy link
Contributor

@AleksanderGondek AleksanderGondek left a comment

Choose a reason for hiding this comment

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

I would like to suggest an shift in approach, which would take into consideration the hermeticity and configurability of the solution - I strongly feel, that rules_nixpkgs should be a ‘poster child’ for such approach and should shy away from hacks relying on host configuration / global environments etc.

Perhaps nixpkgs_package should contain two additional attributes:
Use_remote_nix_builder - boolean, which indicates if remote nix builder is to be used
Remote_nix_builder_config - a dictionary, containing all the configuration needed for the builder to properly run

In that way, we retain the composability of the solution, we do not break hermeticy and are not relying on global system state by default.

# are there issues with this?
# another idea is to have config_settings, and have attributes for this in nixpkgs_package
# but that seems clunkier (have to modify rules, introduce some non-building related logic to rules)
ENV_NIX_REMOTE = 'RULES_NIXPKGS_NIX_REMOTE'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is de-facto a hidden, implict flow control.
Explicit is nearly always better than implicit, especially when dealing with Bazel rules which should clearly define it’s input and output.

Copy link
Member

Choose a reason for hiding this comment

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

If you use an env-var, then make sure to add it to the environ parameter of repository_rule so that Bazel knows when to invalidate and re-run the rule.
This is particularly relevant for this use-case, where failing to re-run the rule after enabling Nix-remote might cause missing store paths on the remote end.


Whether to use an env-var or parameters - we can consider the desired use-cases to see which is better suited.

Parameters provide explicit, checked-in configuration, that will be consistent for all contributors. But, they are difficult to change temporarily, e.g. through a command-line flag such as --config.

Parameters provide fine-grained, per repository rule control. But, they are difficult to set or override globally. E.g. if I need to work offline on transit and want to disable remote builds temporarily.

My understanding is that remote build configuration is something that should be consistent across all instances of nixpkgs_package, to avoid missing remote Nix store paths on individual packages where the parameter was forgotten (e.g. a transitive dependency). And that it is something that we may want to enable or disable via command-line flags or similar configuration, e.g. for temporary offline builds, or different developer and CI configuration.

Viewed that way, env-vars seem closer to what we want.

output_path = exec_result.stdout.splitlines()[-1]

# TODO[GL]: use nix provided ssh?
ssh_path = repository_ctx.which("ssh")
Copy link
Contributor

@AleksanderGondek AleksanderGondek Jun 15, 2022

Choose a reason for hiding this comment

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

Perhaps path to ssh should come from configuration attribute and by default be taken from nix?
Or maybe we can drop ssh'ing all together? Let remote builder worry about caching (or lack of there of)

output_path = exec_result.stdout.splitlines()[-1]

# TODO[GL]: use nix provided ssh?
ssh_path = repository_ctx.which("ssh")
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be capability to control certificates / root certificate / ssh config which is used for this operations (otherwise we are 'escaping to host', breaking hermetcity.

repository_ctx.report_progress("Remote-building Nix derivation")
exec_result = execute_or_fail(
repository_ctx,
[nix_build_path, "--store", nix_store] + expr_args,
Copy link

Choose a reason for hiding this comment

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

imho it makes sense to add --eval-store auto.

(...) which instructs Nix to use a specific Nix store during the evaluation phase (before starting the build). It should be possible to set the evaluation phase to a remote store too, but in practice it makes most sense to always set it to auto.
nixbuild.net documentation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely, figured out this one out afterwards :-).

repository_ctx.attr.attribute_path,
),
quiet = repository_ctx.attr.quiet,
timeout = 10000,
Copy link

Choose a reason for hiding this comment

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

You're using the same timeout value in three places across this PR. I'd suggest either declaring a new variable or re-using the one which is already specified within this file.

@layus
Copy link
Collaborator

layus commented Jun 16, 2022

@AleksanderGondek What about a configurable nix toolchain ? Or something simmilar adapted to external repositories ? Ultimately this nix build sequence of actions should move to a proper wrapper script instead of this starlark spaghetti.

The current design tries to mimic the remote cache and execution setup, where the remote endpoints are defined in .bazelrc. With env vars, you can also set the remote nix endpoint in .bazelrc, which is pretty nice.

Having to specify it in each and every nixpkgs_package seems pretty bad to me. I know you can wrap the repo rule, but the wrapper will not be used everywhere. I think that global state is excatly what we want. Every call, nested inside nix_go setup, nix_python setup or deep inside dependencies of dependencies should inherit the workspace defaults.

@AleksanderGondek
Copy link
Contributor

@layus I think that nix toolchain is a solution long-due :) I really like the idea!

@layus
Copy link
Collaborator

layus commented Jun 17, 2022

But we cannot use toolchains in repository_rules, right ? Oh dear, mind is stuck on Friday.

# are there issues with this?
# another idea is to have config_settings, and have attributes for this in nixpkgs_package
# but that seems clunkier (have to modify rules, introduce some non-building related logic to rules)
ENV_NIX_REMOTE = 'RULES_NIXPKGS_NIX_REMOTE'
Copy link
Member

Choose a reason for hiding this comment

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

If you use an env-var, then make sure to add it to the environ parameter of repository_rule so that Bazel knows when to invalidate and re-run the rule.
This is particularly relevant for this use-case, where failing to re-run the rule after enabling Nix-remote might cause missing store paths on the remote end.


Whether to use an env-var or parameters - we can consider the desired use-cases to see which is better suited.

Parameters provide explicit, checked-in configuration, that will be consistent for all contributors. But, they are difficult to change temporarily, e.g. through a command-line flag such as --config.

Parameters provide fine-grained, per repository rule control. But, they are difficult to set or override globally. E.g. if I need to work offline on transit and want to disable remote builds temporarily.

My understanding is that remote build configuration is something that should be consistent across all instances of nixpkgs_package, to avoid missing remote Nix store paths on individual packages where the parameter was forgotten (e.g. a transitive dependency). And that it is something that we may want to enable or disable via command-line flags or similar configuration, e.g. for temporary offline builds, or different developer and CI configuration.

Viewed that way, env-vars seem closer to what we want.

timeout = 10000,
)

nix_path = repository_ctx.which("nix")
Copy link
Member

Choose a reason for hiding this comment

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

which could return None, which should be handled to provide a reasonable error message. I think the lookup for nix-build already has some relevant logic around it.

@aherrmann
Copy link
Member

@layus

it is hard to specify the ENV_NIX_REMOTE_KEY_FILE when it is located in the repository. You need to expand %workspace% and bazel will not do that for environment variables, so you need wrapper script AFAIK.

Indeed, this is a case where a parameter would serve better than an env-var.

But we cannot use toolchains in repository_rules, right ? Oh dear, mind is stuck on Friday.

Indeed, there are no toolchains in repository rules. That said, one can emulate something like it, at least to a certain extent.
E.g. rules_haskell's use_stack can be used to globally override the stack binary used. Note, it's sensitive to ordering!
Or, rules_haskell's handling of stack update ensures that stack update is called exactly once across multiple repository rules (unless we only read from a lock file).

@AleksanderGondek
Copy link
Contributor

AleksanderGondek commented Jun 20, 2022

@aherrmann @layus

First of all - thank you for taking time to creating the change and taking time to listen to feedback. My comments are coming from the place of deep admiration and care forrules_nixpkgs and I hope I am succeeding in conveying my concerns in fault-free manner :)

Secondly - I am afraid I quite cannot grasp the reason as to why the environment variables are considered adequate in the context of this change. Allow me to expand upon my concerns / point of view (and if I am missing out on some information / plainly wrong - please, just say so):

  1. The main functionality introduced by the change is to allow rules_nixpkgs to offload nix build(s) to remote hosts via mechanisms, which are native to nix, not Bazel.
  2. The distrubuted nix-build capability is realying heavily on proper ssh configuration
  3. As far as Bazel is concerned, the packages prepared by rules_nixpkgs run in “remote mode”, are procured on Bazel client host (the RBE does not benefit directly from this feature, unless runners have some special configuration made)
  4. (Future) We might want to possibly utilize nix substitutors mechanism, to increase the efficiency of the solution

Bazel - in principle and design - wants to describe everything a project needs to build itself. This have multiple advantages (and disadvantages ;) ), one of which is - onboarding new developers / CI executors / general ‘consumers’ of build process is made significantly easier and less error prone, as you have explicitly stated requirements in the WORKSPACE.
Although, this frequently does not hold true in context of dependencies that traditionally come from the OS (python, glibc, etc.), it is an idea the Bazel strives for and rules_nixpkgs is a great boon to it (although, not without it’s own issues).

Now to the part, where I start to complain about the usage of environment variables for configuration of remote nix-build commands (or lack of them): it will introduce a significant surface for misconfiguration and lack of synchronization between users of the Bazel project.

At the moment, as long as you have nix installed, you can have your rules_nixpkgs configured within realm of Bazel knowledge*. With the change - the same Bazel Workspace, with nix installed, with the same command being run, may run properly on one machine and fail miserably on another. The key to proper build will be relegated to knowledge that one has to set env variables and have some ssh keys, taken from somewhere and enable somehow.
Therefore, we would be taking yet another step further from explicit dependencies in Bazel, to some implicit conditions that change the outcome of the build. I feel it is a step in the direction opposite Bazel and nix are guiding us towards, a regression.

I also think, that the very same argument that I have seen raised in proposition of using environment variables, could be made if we were implementing what is currently a repository attribute of a nixpkgs_package rule - why explicitly equip every nix package with a specific nixpkgs definition, if more often than not, we are doing the same definition en masse? It far more easy to read it from environment variable. It is easier to write code for it.
Alas, this is not the way it implemented and I think that decision is obviously something that feels correct in context of Bazel architecture.

Then there is also the matter of further configurability, extendability and sensible feedback to user: usage of environment variables make those unwiedly in very short span of features to implement or use-cases to cover.

Third - to give a constructive suggestion: Perhaps a better alternative to using raw environment variables, would be to:

  1. Introduce new attribue to nixpkgs_pkgs, which points towards an external repository target of a default name (“@io_tweag.nix.remote//:cfg”)
  2. Said target can be a configuration file (or something even wilder) deciding if and how nix-build remote should be used
  3. Said target could be created during “rules_nixpkgs_dependencies” macro evaluation but if - and only if - it does not exists before, defaulting to “do not use remote nix building” setting.

Therefore we would have:

  • Non-breaking change for any previous usage
  • Capability to configure nix-build “en-masse” for every nixpkgs_pakcage
  • Capability to configure nix-build on per nixpkgs basis
  • Retain the ability for the Bazel to have the faintest idea what it needs

Please let me know your thoughts - I would be trilled to prepare a PoC / do some pair-coding if that would help!

@googleson78
Copy link
Contributor Author

Chiming in as a newbie, I agree with the feelings and assessments that @AleksanderGondek has. I think it's also important to be able to switch remote builds on/off for everything at once for basically the reasons @aherrmann outlined.

The solution proposed by Aleksander seems like it's almost there - I can try to implement it. One thing that would be nice but I'm not sure how to do with that approach is to be able to configure through the CLI (or more commonly in .bazelrc.local) turning off/on remote builds easily. Perhaps a selecting on a config_setting in the external repository's BUILD could work, but I'm fuzzy on the details/if it's actually possible before I've started to implement it.

@aherrmann
Copy link
Member

I consider the question of env-var or not an implementation detail. What I consider important is the following:

  1. I'd like to be able to switch off remote builds temporarily, e.g. if I need to work offline, or need to disable all remote caching/execution to debug e.g. a reproducibility issue. To that end I'll need some global switch, e.g. a CLI flag, so that I don't need to go through all occurrences of nixpkgs_package to disable remote execution on each.
  2. The configuration needs to be consistent to avoid missing remote Nix store paths. To that end I'll need some shared, global configuration.

That said, at least for the temporary switch, an env-var paired with --repo_env seems like the most straight forward approach. As for the Nix remote URI and credentials configuration. Sure, some kind of dedicated repository rule that nixpkgs_package depends on could do the trick. However, to ensure consistency it would have to be an implicit dependency.


@googleson78

Perhaps a selecting on a config_setting in the external repository's BUILD could work, but I'm fuzzy on the details/if it's actually possible before I've started to implement it.

Unfortunately, Bazel's configuration features, like select, user defined build settings, etc. are unavailable in repository rules, because they are executed in the loading phase before such configuration is resolved. See here about phases, when select is available is pointed out here for example.

Env-vars are a way to achieve command-line flag configuration with repository rules. Notably, these env-vars don't have to be set manually by the user in their environment. Instead, one can e.g. set them in .bazelrc, for example daml did this for the configuration of the Scala version.

build:scala_2_12 --repo_env=DAML_SCALA_VERSION=2.12.14
fetch:scala_2_12 --repo_env=DAML_SCALA_VERSION=2.12.14
query:scala_2_12 --repo_env=DAML_SCALA_VERSION=2.12.14
sync:scala_2_12 --repo_env=DAML_SCALA_VERSION=2.12.14

To be used as bazel build --config scala_2_12 .... Yes, the duplication is very annoying. Maybe one can use common instead, i.e.

common:scala_2_12 --repo_env=DAML_SCALA_VERSION=2.12.14

in the Daml example.


@AleksanderGondek

I also think, that the very same argument that I have seen raised in proposition of using environment variables, could be made if we were implementing what is currently a repository attribute of a nixpkgs_package rule - why explicitly equip every nix package with a specific nixpkgs definition, if more often than not, we are doing the same definition en masse?

I don't agree with that view - I view the two as different kinds of configurations. The nixpkgs revision has meaning w.r.t. what to build and import, i.e. which version of which tool or library. If another contributor builds their project with a different nixpkgs revision, then I can't expect them to get the same results as I do.

But, the remote configuration has meaning w.r.t. how to build it, i.e. it's operational. In that regard it's similar to a remote cache configuration. In a properly configured, hermetic project I expect to get the same results no matter if I use remote cache or not, it'll just take longer without. Remote execution has a similar quality to it. Of course, it's a bit more complicated, as it can also provide access to more platforms. But, at least in the case of same platform builds, it should behave the same.

@benradf
Copy link
Contributor

benradf commented Jul 17, 2023

Closing as there have been no changes for over a year now. See Support remote execution with rules_nixpkgs for the latest on this topic.

@benradf benradf closed this Jul 17, 2023
@googleson78
Copy link
Contributor Author

googleson78 commented Jul 17, 2023

Hey @benradf, thanks!

Is that link intended to be public, or is it intended entirely for tweagers? Currently it leads to a private Tweag repo, so it's a bit confusing for people who don't have access to it.

@benradf
Copy link
Contributor

benradf commented Jul 17, 2023

Hey @googleson78, good to hear from you again!

Thanks for letting me know the link is private - my mistake. I've changed it to link to the public issue in this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants