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

Replace nixpkgs_cc_configure() with a toolchain #128

Merged
merged 21 commits into from
Nov 30, 2020
Merged

Conversation

aherrmann
Copy link
Member

@aherrmann aherrmann commented May 11, 2020

Depends on #132
Closes #88

This introduces a new repository rule nixpkgs_cc_configure_hermetic which is meant to replace nixpkgs_cc_configure. The latter is implemented by calling Bazel's builtin CC toolchain autodetection with the binary paths overriden to point to Nix provided tools. This is more hermetic than Bazel's default behavior in that the tools are not just picked from $PATH. However, it is not hermetic. Bazel's CC toolchain autodetection is influenced by environment variables, and it shells out to the provided cc binary in the context of a repository rule to determine system include paths which is also affected by environment variables. Concretely, this caused cache misses due to include paths provided by a nix-shell changing the cache keys of build actions depending on the CC toolchain.

nixpkgs_cc_configure_hermetic on the other hand uses binaries determined by a Nix derivation and performs additional configuration such as determining system include paths within a Nix build, i.e. within Nix's own sandbox. The results are stored in a text file CC_TOOLCHAIN_INFO which is then parsed in a Bazel repository rule. From there on the toolchain configuration follows the example of Bazel's builtin CC toolchain for Unix: https://github.com/bazelbuild/bazel/blob/0f4c498a270f05b3896d57055b6489e824821eda/tools/cpp/unix_cc_configure.bzl#L310.

Users can override the Nix provided compiler using the nix_file or nix_file_content and attribute_path arguments. The corresponding Nix derivation should produce the required tools, e.g. bin/cc. The implementation of this feature required a way to pass the path to a Nix file (the nix_file argument) to the nixopts attribute of nixpkgs_package from within a macro. This is difficult to do correctly without repository_ctx available. Instead, of trying to predict this path from within a macro a more robust approach is to define location expansion for the nixopts attribute using $(location ...) patterns similar to ctx.expand_location. This feature is implemented in #132.

@aherrmann aherrmann force-pushed the cc-toolchain branch 3 times, most recently from 86538de to 2259c1c Compare May 14, 2020 08:17
@aherrmann aherrmann force-pushed the cc-toolchain branch 4 times, most recently from 5351c13 to 1d84fb5 Compare May 15, 2020 13:13
@aherrmann aherrmann force-pushed the cc-toolchain branch 2 times, most recently from b1b3e6f to b92b50e Compare May 18, 2020 09:36
@aherrmann aherrmann closed this May 18, 2020
@aherrmann aherrmann reopened this May 18, 2020
@aherrmann aherrmann changed the title [WIP] Replace nixpkgs_cc_configure() with a toolchain Replace nixpkgs_cc_configure() with a toolchain May 18, 2020
@aherrmann aherrmann marked this pull request as ready for review May 18, 2020 10:11
@aherrmann aherrmann requested review from mboes, Profpatsch and guibou and removed request for mboes May 18, 2020 10:11
@@ -70,6 +78,54 @@ nixpkgs_local_repository = repository_rule(
def _is_supported_platform(repository_ctx):
return repository_ctx.which("nix-build") != None

def _expand_location(repository_ctx, string, labels, attr = None):
"""Expand `$(location label)` to a path.
Copy link
Member Author

Choose a reason for hiding this comment

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

To be consistent with Bazel's builtin location expansion this should error on unescaped instances of $() that are not $(location ...).

Copy link
Contributor

@guibou guibou left a comment

Choose a reason for hiding this comment

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

This is a huge change, LGTM with a diagonal read and it does not break anything for me. But take this with a grain of salt ;)

This being said, nixpkgs_cc_configure may still be necessary in some context. On a client codebase, we are using a toolchain (with a setup similar to the one involved here) but still observes that bazel is still looking for a system toolchain for some tasks (this is a bootstrap problem).

So I do think that nixpkgs_cc_configure_hermetic is not a better nixpkgs_cc_configure, just that it solves a different problem.

Side question, can this toolchain configuration made more composable so people who are already setting up a CC toolchain may just inherit part of it so their CC toolchain will become Haskell compatible?

@aherrmann
Copy link
Member Author

This being said, nixpkgs_cc_configure may still be necessary in some context. On a client codebase, we are using a toolchain (with a setup similar to the one involved here) but still observes that bazel is still looking for a system toolchain for some tasks (this is a bootstrap problem).

In cases where Bazel looks at local_config_cc, e.g. because it is hard-coded someplace, you can use nixpkgs_cc_configure_hermetic(name = "local_config_cc", ...). This overrides the builtin CC toolchain just as nixpkgs_cc_configure would, but is more hermetic.

@aherrmann
Copy link
Member Author

Side question, can this toolchain configuration made more composable so people who are already setting up a CC toolchain may just inherit part of it so their CC toolchain will become Haskell compatible?

Can you describe the use-case you have in mind? nixpkgs_configure_cc_hermetic allows users to provide their own Nix expression, so it should be very flexible on that side.

Note, this toolchain is not a prerequisite for rules_haskell. E.g. with rules_haskell with GHC from bindist you can just use Bazel's builtin CC toolchain. It's just that a nixpkgs provided GHC and Bazel's builtin CC toolchain don't work well together. However, if you have another means to provide a CC toolchain that works with a nixpkgs GHC, then you don't have to use this one.

This way the presence of the flag remains user configurable through the
nix_file or nix_file_content attribute.
@aherrmann
Copy link
Member Author

aherrmann commented Nov 24, 2020

@micahcc Thank you for testing this! Indeed, I can reproduce the issue. The MacOS version of the toolchain fixes this with additional flags using makeWrapper. Though there seems to be a difference between MacOS and Linux regarding linker flags.

Unfortunately, always using c++ instead of cc would break compilation of regular C code that happens to be invalid C++. However, the Nix clang wrapper picks up the -x c++ flag. So, adding that on C++ compilation avoids the missing include paths. I've pushed the necessary changes here. Does that fix the issue for your use-case?

@aherrmann
Copy link
Member Author

I've been able to reproduce the clang issue on Linux with the previous nixpkgs_cc_configure implementation as well. Meaning, this is not a regression introduced by this change. In that case one has to point the envvar CC to the Nix provided clang since the builtin autoconfigure cc toolchain otherwise uses gcc on Linux.

To move forward with this I'll merge this PR and open a separte issue and PR for the clang issue.

@aherrmann aherrmann added the merge-queue merge on green CI label Nov 30, 2020
@mergify mergify bot merged commit 90d141a into master Nov 30, 2020
@mergify mergify bot deleted the cc-toolchain branch November 30, 2020 12:43
@mergify mergify bot removed the merge-queue merge on green CI label Nov 30, 2020
@@ -1 +1,7 @@
build --host_platform=@io_tweag_rules_nixpkgs//nixpkgs/platforms:host
build --crosstool_top=@nixpkgs_config_cc//:toolchain
# Using toolchain resolution can lead to spurious dependencies on
# `@local_config_cc//:builtin_include_directory_paths`. This needs to be
Copy link
Member

Choose a reason for hiding this comment

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

@aherrmann is there a ticket tracking this? Once this flag is enabled, do I understand right that --crosstool_top will no longer be necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

@aherrmann is there a ticket tracking this?

I've looked into the issue in the daml repo once more. It looks like this is a rules_haskell issue: tweag/rules_haskell#1467

However, in general bazelbuild/bazel#1258 suggests that there is still ongoing work in Bazel in this area. And bazelbuild/bazel#12712 also seems related.

Once this flag is enabled, do I understand right that --crosstool_top will no longer be necessary?

Correct, assuming that all involved Starlark code is compatible with cc toolchain resolution. E.g. any direct reference to ctx.attr._cc_toolchain breaks this and causes a dependency on --crosstool_top.

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.

Replace nixpkgs_cc_configure() with a toolchain
6 participants