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

doc: add a guide covering basic rules_nixpkgs use + Nix prerequisites #294

Merged
merged 16 commits into from
Apr 17, 2023

Conversation

evertedsphere
Copy link
Contributor

@evertedsphere evertedsphere commented Dec 20, 2022

Add an initial version of a rules_nixpkgs guide (fix #269)

The target audience is Bazel users new to Nix, so I've taken care to not assume Nix-related knowledge and to introduce the minimum amount of information required to be able to start using the ruleset.

For the purposes of this version, we only target Linux; #371 tracks macOS support.

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you! I did a first pass and added some comments.

guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
@evertedsphere evertedsphere force-pushed the es/guide branch 5 times, most recently from ef4c5fc to fb5b8f8 Compare January 27, 2023 13:11
@evertedsphere evertedsphere marked this pull request as ready for review January 27, 2023 13:34
Copy link
Member

@mrkkrp mrkkrp left a comment

Choose a reason for hiding this comment

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

I left some comments. Great job, I enjoyed going through this! I feel like the beginning of the guide is more polished and you probably spent more time working on it and its wording. There is a noticeable difference in sophistication of writing, which gets simpler and more sketchy towards the end. It would be nice to bring the second half to the same level as the first.

guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you, it's a nice read, good work!
I've added a couple comments.

guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated

TODO: does use flake work with vanilla direnv? nix-direnv is just an improvement over that, i believe
TODO: use the same nixpkgs.nix for nixpkgs_git_repository and shell.nix
TODO: is the flake shell pure? because if this is run on a non-nixos system, and /usr/bin/gcc continues to be visible, that messes things up
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's not, at least not by default.
Generally, the correctness of the described Nix+Bazel setup should not hinge on the purity of the Nix shell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the Nix shell isn't pure, some of the stuff I've written about @local_config_cc not working when in the shell and having that extra insulation from accidentally using the non-hermetic toolchain becomes false, doesn't it?

Copy link
Member

Choose a reason for hiding this comment

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

True, best to double check with #nix. I don't see --pure flag similar to the nix-shell one on nix develop, only --impure, but IIUC that is about Nix evaluation rather than the resulting shell environment.

guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
Comment on lines +268 to +512
It is entirely possible to make the use of Nix optional in a Bazel setup. To do
so, we use `bazelrc` configs to only enable Nix-specific configuration when a
`--config` flag is provided, like so:
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, the WORKSPACE file needs to configure both a nix provided toolchain and a non-nix alternative, e.g. the default bindist. It is important that the nix provided one is registered first, meaning before the non-nix one. The reason is that the nix one is marked with an additional execution platform constraint, which is provided by the nixpkgs platform defined with the --host_platform flag. However, the default bindist toolchain typically has no corresponding platform constraint indicating a non-nix platform, and would therefore also be a valid choice on the nix platform. If the nix toolchain is defined after the non-nix when, then it would never be selected by Bazel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hadn't thought about that, I'll make a note of this.

As for defining a non-Nix toolchain, the rules_cc_toolchains() call takes care of that, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a note about this; could you check that it makes sense? (I wasn't really sure what the "default bindist" specifically referred to.)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, yes, that makes sense. The exact shape of the "default bindist" depends on the language. With cc its implicit since it's builtin to Bazel. With other languages it would be your rules_XXX_toolchains call, or similar that sets this up, e.g. here for Go.

guide.md Show resolved Hide resolved
@evertedsphere evertedsphere force-pushed the es/guide branch 5 times, most recently from c5b2877 to f9fd614 Compare February 3, 2023 14:04
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
@aherrmann
Copy link
Member

IIUC https://github.com/tweag/rules_nixpkgs_cc_template is the worked example for this user-guide, correct? Could you pull it into the rules_nixpkgs repository and add a test for it to CI? (Probably best in its own PR). rules_haskell does the same with its tutorial.

That makes sure that the example is tested in CI, and that changes to the example can happen along-side the relevant changes to the guide and vice-versa.

aherrmann referenced this pull request in tweag/rules_nixpkgs_cc_template Feb 7, 2023
aherrmann referenced this pull request in tweag/rules_nixpkgs_cc_template Feb 7, 2023
guide.md Outdated
it wants and will fail to work.

Now developers on different platforms can enter a development environment by
running `nix develop`, and this environment will be identical across platforms.
Copy link
Member

Choose a reason for hiding this comment

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

this environment will be identical across platforms

Nit: I think I know what you're getting at, but it's a bit ambiguous. Is the user-guide only targeting Linux or is it also expected to work on MacOS? The reason I'm asking: Arguably "across platforms" would also mean across Linux and MacOS. But, in that case the environment is not identical, on MacOS it'll include Darwin executables instead of Linux ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this to "consistent" instead of "identical".

As for the first question: I wrote the guide with Linux in mind. Not sure if it would work elsewhere, but perhaps we can investigate macOS compatibility and try to make it work if we have a reasonable number of macOS users.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Focusing on Linux first seems perfectly fair and I'm happy to leave MacOS for follow-up work. In that case it should just be stated somewhere that this guide is only targeting Linux and we'd also need an issue on rules_nixpkgs to track the MacOS support.

guide.md Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
guide.md Outdated Show resolved Hide resolved
@evertedsphere evertedsphere force-pushed the es/guide branch 6 times, most recently from cf250bd to 371c842 Compare February 16, 2023 15:32
evertedsphere and others added 10 commits April 13, 2023 15:38
If direnv creates a .direnv directory to link in Nix store paths then we
need to instruct Bazel to ignore that tree. Otherwise, commands like
`bazel build //...` lead to very confusing errors about failing to load
packges underneath `.direnv/...`.
@evertedsphere evertedsphere changed the title guide: init doc: add a guide covering basic rules_nixpkgs use + Nix prerequisites Apr 13, 2023
@evertedsphere
Copy link
Contributor Author

Should be mergeable now. Let me know if any more changes are necessary.

Copy link
Contributor

@benradf benradf left a comment

Choose a reason for hiding this comment

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

Looks good!

@benradf benradf added the merge-queue merge on green CI label Apr 17, 2023
@mergify mergify bot merged commit 8a4ab72 into master Apr 17, 2023
@mergify mergify bot deleted the es/guide branch April 17, 2023 10:22
@mergify mergify bot removed the merge-queue merge on green CI label Apr 17, 2023
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you! Looks great!

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.

Create user-guide documentation
6 participants