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

Implement a nicer repository interface for nixpkgs #387

Closed
wants to merge 2 commits into from

Conversation

lunaris
Copy link
Collaborator

@lunaris lunaris commented Aug 6, 2018

Problem

Depending on the end goal, the set of steps required to set up rules_haskell with a GHC from nixpkgs (the recommended use case) is a bit involved. At present, after loading rules_haskell, one must at least:

  • In WORKSPACE, bring in a GHC from nixpkgs using nixpkgs_package (from rules_nixpkgs).
  • In a BUILD, call haskell_toolchain to instantiate a toolchain using the nixpkgs GHC.
  • In WORKSPACE, call register_toolchain to register the defined toolchain.

This is not that bad and likely comparable to most rules_language packages for languages other than those supported natively. However, the situation degenerates when one wants to use packages from Hackage using Hazel. At this point, one must:

  • Tweak the above nixpkgs_package call to use Hazel's custom BUILD.ghc file, which adds some exports (and also calls haskell_toolchain).

  • Currently undocumented but I think required in many, if not all cases -- also pull a version of GCC and some of its libraries from nixpkgs, along with patchelf, and use these to ensure that the nixpkgs GHC produces binaries linked against the Nix store and not the host system. This requires using some bits and pieces that are now exported in the Hazel repository (e.g. cc_configure_custom).

Proposal

Even disregarding the possibility that Hazel and rules_haskell will be merged at some point, most of the steps taken to get Hazel working are sensible in the context of rules_haskell alone. Moreover, I think we could do better to ensure a "pit of success" for the nixpkgs use case if it's the recommended path. Consequently, this PR proposes the following:

  • A haskell_nixpkgs_toolchain macro which is intended to the be the one-stop shop for setting up a nixpkgs-backed GHC/Haskell toolchain. Usage might be as follows:
nixpkgs_git_repository(
    name = "nixpkgs",
    revision = "ee80654b5267b07ba10d62d143f211e0be81549e",
)

haskell_nixpkgs_toolchain(
    name = "nixpkgs",
    repository = "@nixpkgs",
    ghc_version = "8.2.2",
    compiler_flags = [
        "-Wall",
        "-Werror",
    ],
)

This will bring in GHC, c2hs etc. from nixpkgs and perform the necessary GCC/patching dance to make sure that the binaries built are pinned to the Nix store. It will also call haskell_toolchain and register_toolchain with the correct arguments. The macro's name attribute is used to namespace everything so this will create targets nixpkgs_ghc, nixpkgs_c2hs etc. though these can be overridden with kwargs like ghc_package_name = "ghc" (which e.g. is hard-coded into older Hazel versions).

Pros

  • "Just works" I think for the nixpkgs case -- binaries are now linked against the Nix store (in a "mostly-static" sense), which they presumably should be.
  • Pulls out generic code from Hazel, which should enable deletion of this code from Hazel.
  • haskell_toolchain is still exported if users want something more bespoke.

Cons/not currently done

  • Only parameters at this point are GHC version and compiler flags but perhaps we could/should add more. I've refrained here because this PR is also a request for comment as much as anything. Documentation is also required.
  • The PR as it currently stands won't build because I put some of the patchelf code into rules_nixpkgs -- does this sound sensible? If not and we want to keep rules_nixpkgs super barebones I'll pull it back down into rules_haskell, though I think there could be a use case for it:
def _nixpkgs_patched_solib_impl(ctx):
    src = ctx.file.src
    if ctx.attr.is_darwin:
        output = ctx.actions.declare_file("lib" + ctx.attr.name + ".dylib")
        ctx.actions.run_shell(
            inputs = [src],
            outputs = [output],
            progress_message = "Patching dylib %s" % ctx.label,
            command = "cp {src} {out}".format(
                src = src.path,
                out = output.path,
                ),
            )
    else:
        output = ctx.actions.declare_file("lib" + ctx.label.name + ".so")
        ctx.actions.run_shell(
            inputs = [src, ctx.executable._patchelf],
            outputs = [output],
            progress_message = "Patching solib %s" % ctx.label,
            command = ("cp {src} {out} && chmod +w {out} && "
                      + "{patchelf} --set-soname {outname} {out}").format(
                        src = src.path,
                        out = output.path,
                        patchelf = ctx.executable._patchelf.path,
                        outname = output.basename,
                        ),
            )

    return [DefaultInfo(files=depset([output]))]

_nixpkgs_patched_solib = rule(
    _nixpkgs_patched_solib_impl,
    attrs = {
        "src": attr.label(allow_files = True, single_file = True),
        "_patchelf": attr.label(
            default = Label("@patchelf//:patchelf"),
            executable = True,
            cfg = "host",
            ),
        "is_darwin": attr.bool(),
        },
    )

def nixpkgs_patched_solib(name, lib_name):
    _nixpkgs_patched_solib(
        name = name,
        src = native.glob(["lib/lib" + lib_name + ext for ext in [".so", ".dylib"]])[0],
        is_darwin = select({
            "@bazel_tools//src/conditions:darwin": True,
            "//conditions:default": False,
            }),
        )

@lunaris lunaris requested review from mboes and judah August 7, 2018 07:39
@lunaris
Copy link
Collaborator Author

lunaris commented Aug 9, 2018

Do we have any thoughts on this? I'm finding it quite useful but if we think it's outside the scope of this repo I can find another place for it.

@mboes
Copy link
Member

mboes commented Aug 9, 2018

Sorry I haven't had a chance to look at this yet. I have pretty much no experience with Hazel. @mrkkrp perhaps you could have a first look?

@mrkkrp
Copy link
Member

mrkkrp commented Aug 10, 2018

I'll take a look tomorrow.

@mrkkrp
Copy link
Member

mrkkrp commented Aug 11, 2018

This looks sensible to me and matches quite well how rules_haskell + Hazel is used in production.

@judah
Copy link
Collaborator

judah commented Aug 11, 2018

FYI, I recently pushed an improvement to Hazel so that the compiler is wired up correctly on MacOS: https://github.com/FormationAI/hazel/pull/39/files

@lunaris
Copy link
Collaborator Author

lunaris commented Aug 16, 2018

Trying to get this passing the tests but having trouble running them locally. Running:

nix-shell --pure --run "./run_tests.sh"

gives:

ERROR: /.../external/io_bazel_rules_go/go/tools/builders/BUILD.bazel:138:1: in go_tool_binary rule @io_bazel_rules_go//go/tools/builders:stdlib:
Traceback (most recent call last):
        File "/.../external/io_bazel_rules_go/go/tools/builders/BUILD.bazel", line 138
                go_tool_binary(name = 'stdlib')
        File "/.../external/io_bazel_rules_go/go/private/rules/binary.bzl", line 52, in _go_binary_impl
                go_context(ctx)
        File "/.../external/io_bazel_rules_go/go/private/context.bzl", line 237, in go_context
                _get_go_binary(context_data)
        File "/.../external/io_bazel_rules_go/go/private/context.bzl", line 216, in _get_go_binary
                fail("Could not find go executable in...")
Could not find go executable in go_sdk
ERROR: Analysis of target '//docs:lint_buildifier' failed; build aborted: Analysis of target '@io_bazel_rules_go//go/tools/builders:stdlib' failed; build aborted

Am I missing something obvious here? nix-shell --run go tells me that go is definitely there in the environment.

@mrkkrp
Copy link
Member

mrkkrp commented Aug 22, 2018

Works for me locally. It fails with the same lint-related error as on CI. What OS are you on?

@lunaris
Copy link
Collaborator Author

lunaris commented Aug 29, 2018

I'm on Ubuntu 16.04 using Nix 2.0.4. Neither master nor my branch run the tests, even after bazel clean. Hm.

@Profpatsch
Copy link
Contributor

Profpatsch commented Aug 29, 2018

I'm on Ubuntu 16.04 using Nix 2.0.4.

go_sdk downloads their own go ELF binary.
See bazel-contrib/rules_go#1611, I’m still in the process of trying it out.

Update: It works if you follow the nix code examples from the issue.

@lunaris
Copy link
Collaborator Author

lunaris commented Oct 9, 2018

I'm going to close this for the time being as I think #442 is a superior working direction (harnessing Nix and thus obviating the need for clever toolchain tricks to try and mitigate custom builds with Hazel).

@lunaris lunaris closed this Oct 9, 2018
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.

5 participants