Refactoring: remove visible-binaries

To precisely control what binaries are available during compilation,
we used to create a single directory called `visible-binaries`, in
which we put all the binaries (ghc, haddock, etc) and shell
commands (cat, mkdir, etc) that we needed. There was another point to
this: build actions could call any binary and that binary could in
turn exec any other binary, since `visible-binaries` was in the
`PATH`.

In this commit, we do away with that. The reason is that there are
downsides to this approach:

* there is little value in trying to be more hermetic than Bazel is
  when it comes to shell commands;
* all build actions were reusing the same `PATH`, so we were actually
  exposing more binaries than strictly necessary (each action needs
  a different subset);
* the logic to create `visible-binaries` was pretty complicated, and
  is now all gone, hence the net code reduction;
* crucially, the `visible-binaries` is not going to work on Windows,
  because no symlinks on that platform and copying doesn't always work
  (due to limitations of GHC).

With this PR merged in, porting to Windows should now be significantly
easier. The downside is the following *breaking change*: we have lost
the `extra_binaries` field in rules. But that field was introduced for
unusual needs that inline-java has, and was a suspicious feature to
begin with.

Probably fixes #244.

CHANGELOG.md

@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 * The `prebuilt_dependencies` attribute of all haskell rules
   had been deprecated two versions ago and is removed.
   Use `haskell_import` instead (see docs for usage).
+* The `extra_binaries` field is now no longer supported.
 
 ## [0.7] - 2018-12-24
 

haskell/BUILD

@@ -1,9 +1,8 @@
 exports_files(
     glob(["*.bzl"]) + [
         "assets/ghci_script",
-        "private/c2hs_wrapper.sh",
         "private/ghci_repl_wrapper.sh",
-        "private/haddock_wrapper.sh",
+        "private/haddock_wrapper.sh.tpl",
     ],
 )
 

haskell/c2hs.bzl

@@ -29,7 +29,7 @@ def _c2hs_library_impl(ctx):
     args.add([chs_file.path, "-o", hs_file.path])
 
     args.add(["-C-E"])
-    args.add(["--cpp", hs.tools.cc.path])
+    args.add(["--cpp", cc.tools.cc])
     args.add("-C-includeghcplatform.h")
     args.add("-C-includeghcversion.h")
     args.add(["-C" + x for x in cc.cpp_flags])
@@ -48,14 +48,21 @@ def _c2hs_library_impl(ctx):
     ]
     args.add(chi_includes)
 
-    hs.actions.run(
+    hs.actions.run_shell(
         inputs = depset(transitive = [
             depset(cc.hdrs),
-            depset([hs.tools.cc, hs.tools.ghc, hs.tools.c2hs, chs_file]),
+            depset([hs.tools.ghc, hs.tools.c2hs, chs_file]),
             depset(dep_chi_files),
         ]),
         outputs = [hs_file, chi_file],
-        executable = ctx.file._chs_wrapper,
+        command = """
+        # Include libdir in include path just like hsc2hs does.
+        libdir=$({ghc} --print-libdir)
+        {c2hs} -C-I$libdir/include "$@"
+        """.format(
+            ghc = hs.tools.ghc.path,
+            c2hs = hs.tools.c2hs.path,
+        ),
         mnemonic = "HaskellC2Hs",
         arguments = [args],
         env = hs.env,
@@ -84,10 +91,6 @@ c2hs_library = rule(
         "src_strip_prefix": attr.string(
             doc = "Directory in which module hierarchy starts.",
         ),
-        "_chs_wrapper": attr.label(
-            allow_single_file = True,
-            default = Label("@io_tweag_rules_haskell//haskell:private/c2hs_wrapper.sh"),
-        ),
         "_cc_toolchain": attr.label(
             default = Label("@bazel_tools//tools/cpp:current_cc_toolchain"),
         ),

haskell/cc.bzl

@@ -22,6 +22,7 @@ load(
 CcInteropInfo = provider(
     doc = "Information needed for interop with cc rules.",
     fields = {
+        "tools": "Tools from the CC toolchain",
         "hdrs": "CC headers",
         "cpp_flags": "Preprocessor flags",
         "compiler_flags": "Flags for compilation",
@@ -121,7 +122,23 @@ def cc_interop_info(ctx):
     # XXX Workaround https://github.com/bazelbuild/bazel/issues/6876.
     linker_flags = [flag for flag in linker_flags if flag not in ["-shared"]]
 
+    tools = {
+        "ar": cc_toolchain.ar_executable(),
+        "cc": cc_toolchain.compiler_executable(),
+        "ld": cc_toolchain.ld_executable(),
+        "cpp": cc_toolchain.preprocessor_executable(),
+    }
+
+    # If running on darwin but XCode is not installed (i.e., only the Command
+    # Line Tools are available), then Bazel will make ar_executable point to
+    # "/usr/bin/libtool". Since we call ar directly, override it.
+    # TODO: remove this if Bazel fixes its behavior.
+    # Upstream ticket: https://github.com/bazelbuild/bazel/issues/5127.
+    if tools["ar"].find("libtool") >= 0:
+        tools["ar"] = "/usr/bin/ar"
+
     return CcInteropInfo(
+        tools = struct(**tools),
         hdrs = hdrs.to_list(),
         cpp_flags = cpp_flags,
         include_args = include_args,

haskell/doctest.bzl

@@ -151,15 +151,26 @@ def _haskell_doctest_single(target, ctx):
             depset([exposed_modules_file]),
             depset(
                 toolchain.doctest +
-                [hs.tools.cat, hs.tools.ghc],
+                [hs.tools.ghc],
             ),
         ]),
         outputs = [doctest_log],
         mnemonic = "HaskellDoctest",
         progress_message = "HaskellDoctest {}".format(ctx.label),
         command = """
-    {doctest} "$@" $(cat {module_list} | tr , ' ') > {output} 2>&1 || rc=$? && cat {output} && exit $rc
-    """.format(doctest = toolchain.doctest[0].path, output = doctest_log.path, module_list = exposed_modules_file.path),
+        {env}
+        {doctest} "$@" $(cat {module_list} | tr , ' ') > {output} 2>&1 || rc=$? && cat {output} && exit $rc
+        """.format(
+            doctest = toolchain.doctest[0].path,
+            output = doctest_log.path,
+            module_list = exposed_modules_file.path,
+            # XXX Workaround
+            # https://github.com/bazelbuild/bazel/issues/5980.
+            env = "\n".join([
+                "export {}={}".format(k, v)
+                for k, v in hs.env.items()
+            ]),
+        ),
         arguments = [args],
         # NOTE It looks like we must specify the paths here as well as via -L
         # flags because there are at least two different "consumers" of the info

haskell/haddock.bzl

@@ -89,6 +89,25 @@ def _haskell_doc_aspect_impl(target, ctx):
         depset([hs.toolchain.locale_archive]) if hs.toolchain.locale_archive != None else depset()
     )
 
+    # TODO(mboes): we should be able to instantiate this template only
+    # once per toolchain instance, rather than here.
+    haddock_wrapper = ctx.actions.declare_file("haddock_wrapper")
+    ctx.actions.expand_template(
+        template = ctx.file._haddock_wrapper_tpl,
+        output = haddock_wrapper,
+        substitutions = {
+            "%{ghc-pkg}": hs.tools.ghc_pkg.path,
+            "%{haddock}": hs.tools.haddock.path,
+            # XXX Workaround
+            # https://github.com/bazelbuild/bazel/issues/5980.
+            "%{env}": "\n".join([
+                "export {}={}".format(k, v)
+                for k, v in hs.env.items()
+            ]),
+        },
+        is_executable = True,
+    )
+
     ctx.actions.run(
         inputs = depset(transitive = [
             set.to_depset(target[HaskellBuildInfo].package_caches),
@@ -105,24 +124,21 @@ def _haskell_doc_aspect_impl(target, ctx):
             set.to_depset(target[HaskellLibraryInfo].source_files),
             target[HaskellLibraryInfo].extra_source_files,
             depset([
-                hs.tools.bash,
                 hs.tools.ghc_pkg,
                 hs.tools.haddock,
-                hs.tools.mktemp,
-                hs.tools.rmdir,
             ]),
             locale_archive_depset,
         ]),
         outputs = [haddock_file, html_dir],
         mnemonic = "HaskellHaddock",
         progress_message = "HaskellHaddock {}".format(ctx.label),
-        executable = ctx.file._haddock_wrapper,
+        executable = haddock_wrapper,
         arguments = [
             prebuilt_deps,
             args,
             ghc_args,
         ],
-        env = hs.env,
+        use_default_shell_env = True,
     )
 
     transitive_html.update({package_id: html_dir})
@@ -140,9 +156,9 @@ def _haskell_doc_aspect_impl(target, ctx):
 haskell_doc_aspect = aspect(
     _haskell_doc_aspect_impl,
     attrs = {
-        "_haddock_wrapper": attr.label(
+        "_haddock_wrapper_tpl": attr.label(
             allow_single_file = True,
-            default = Label("@io_tweag_rules_haskell//haskell:private/haddock_wrapper.sh"),
+            default = Label("@io_tweag_rules_haskell//haskell:private/haddock_wrapper.sh.tpl"),
         ),
     },
     attr_aspects = ["deps"],
@@ -191,11 +207,7 @@ def _haskell_doc_rule_impl(ctx):
         html_dict_copied[package_id] = output_dir
 
         ctx.actions.run_shell(
-            inputs = [
-                hs.tools.cp,
-                hs.tools.mkdir,
-                html_dir,
-            ],
+            inputs = [html_dir],
             outputs = [output_dir],
             command = """
       mkdir -p "{doc_dir}"

haskell/lint.bzl

@@ -80,22 +80,26 @@ def _haskell_lint_aspect_impl(target, ctx):
             set.to_depset(build_info.interface_dirs),
             set.to_depset(build_info.dynamic_libraries),
             depset([e.mangled_lib for e in set.to_list(build_info.external_libraries)]),
-            depset([
-                hs.tools.ghc,
-                hs.tools.cat,
-            ]),
+            depset([hs.tools.ghc]),
         ]),
         outputs = [lint_log],
         mnemonic = "HaskellLint",
         progress_message = "HaskellLint {}".format(ctx.label),
         command = """
-    ghc "$@" > {output} 2>&1 || rc=$? && cat {output} && exit $rc
-    """.format(
+        {env}
+        {ghc} "$@" > {output} 2>&1 || rc=$? && cat {output} && exit $rc
+        """.format(
             ghc = hs.tools.ghc.path,
             output = lint_log.path,
+            # XXX Workaround
+            # https://github.com/bazelbuild/bazel/issues/5980.
+            env = "\n".join([
+                "export {}={}".format(k, v)
+                for k, v in hs.env.items()
+            ]),
         ),
         arguments = [args],
-        env = hs.env,
+        use_default_shell_env = True,
     )
 
     lint_info = HaskellLintInfo(outputs = set.singleton(lint_log))

haskell/private/actions/compile.bzl

@@ -36,8 +36,8 @@ def _process_hsc_file(hs, cc, hsc_flags, hsc_file):
     hs_out = declare_compiled(hs, hsc_file, ".hs", directory = hsc_dir_raw)
     args.add([hsc_file.path, "-o", hs_out.path])
 
-    args.add(["-c", hs.tools.cc])
-    args.add(["-l", hs.tools.cc])
+    args.add(["-c", cc.tools.cc])
+    args.add(["-l", cc.tools.cc])
     args.add("-ighcplatform.h")
     args.add("-ighcversion.h")
     args.add(["--cflag=" + f for f in cc.cpp_flags])
@@ -49,7 +49,7 @@ def _process_hsc_file(hs, cc, hsc_flags, hsc_file):
     hs.actions.run(
         inputs = depset(transitive = [
             depset(cc.hdrs),
-            depset([hs.tools.cc, hsc_file]),
+            depset([hsc_file]),
         ]),
         outputs = [hs_out],
         mnemonic = "HaskellHsc2hs",
@@ -250,7 +250,6 @@ def _compilation_defaults(hs, cc, java, dep_info, srcs, import_dir_map, extra_sr
             set.to_depset(dep_info.dynamic_libraries),
             depset([e.mangled_lib for e in set.to_list(dep_info.external_libraries)]),
             java.inputs,
-            depset([hs.tools.cc]),
             locale_archive_depset,
         ]),
         objects_dir = objects_dir,
@@ -291,7 +290,8 @@ def compile_binary(hs, cc, java, dep_info, srcs, ls_modules, import_dir_map, ext
 
     hs.toolchain.actions.run_ghc(
         hs,
-        inputs = c.inputs + hs.extra_binaries,
+        cc,
+        inputs = c.inputs,
         outputs = c.outputs,
         mnemonic = "HaskellBuildBinary",
         progress_message = "HaskellBuildBinary {}".format(hs.label),
@@ -348,7 +348,8 @@ def compile_library(hs, cc, java, dep_info, srcs, ls_modules, other_modules, exp
 
     hs.toolchain.actions.run_ghc(
         hs,
-        inputs = c.inputs + hs.extra_binaries,
+        cc,
+        inputs = c.inputs,
         outputs = c.outputs,
         mnemonic = "HaskellBuildLibrary",
         progress_message = "HaskellBuildLibrary {}".format(hs.label),

haskell/private/actions/link.bzl

@@ -205,6 +205,7 @@ def link_binary(
     )
     hs.toolchain.actions.run_ghc(
         hs,
+        cc,
         inputs = depset(transitive = [
             depset(extra_srcs),
             set.to_depset(dep_info.package_caches),
@@ -213,7 +214,6 @@ def link_binary(
             depset(dep_info.static_libraries_prof),
             depset([objects_dir]),
             depset([e.mangled_lib for e in set.to_list(dep_info.external_libraries)]),
-            depset(hs.extra_binaries),
         ]),
         outputs = [compile_output],
         mnemonic = "HaskellLinkBinary",
@@ -298,8 +298,7 @@ def link_library_static(hs, cc, dep_info, objects_dir, my_pkg_id, with_profiling
         with_profiling = with_profiling,
     )
     args = hs.actions.args()
-    inputs = ([objects_dir, objects_dir_manifest, hs.tools.ar] +
-              hs.tools_runfiles.ar + hs.extra_binaries)
+    inputs = [objects_dir, objects_dir_manifest]
 
     if hs.toolchain.is_darwin:
         # On Darwin, ar doesn't support params files.
@@ -316,7 +315,7 @@ def link_library_static(hs, cc, dep_info, objects_dir, my_pkg_id, with_profiling
             inputs = inputs,
             outputs = [static_library],
             mnemonic = "HaskellLinkStaticLibrary",
-            command = "{ar} qc $1 $(< $2)".format(ar = hs.tools.ar.path),
+            command = "{ar} qc $1 $(< $2)".format(ar = cc.tools.ar.path),
             arguments = [args],
 
             # Use the default macosx toolchain
@@ -332,7 +331,7 @@ def link_library_static(hs, cc, dep_info, objects_dir, my_pkg_id, with_profiling
             inputs = inputs,
             outputs = [static_library],
             mnemonic = "HaskellLinkStaticLibrary",
-            executable = hs.tools.ar,
+            executable = cc.tools.ar,
             arguments = [args],
         )
 
@@ -405,8 +404,8 @@ def link_library_dynamic(hs, cc, dep_info, extra_srcs, objects_dir, my_pkg_id):
 
     hs.toolchain.actions.run_ghc(
         hs,
+        cc,
         inputs = depset([objects_dir], transitive = [
-            depset(hs.extra_binaries),
             depset(extra_srcs),
             set.to_depset(dep_info.package_caches),
             set.to_depset(dep_info.dynamic_libraries),

haskell/private/context.bzl

@@ -22,7 +22,6 @@ def haskell_context(ctx, attr = None):
     )
 
     env = {
-        "PATH": toolchain.visible_bin_path,
         "LANG": toolchain.locale,
     }
 
@@ -35,8 +34,6 @@ def haskell_context(ctx, attr = None):
         label = ctx.label,
         toolchain = toolchain,
         tools = toolchain.tools,
-        tools_runfiles = toolchain.tools_runfiles,
-        extra_binaries = toolchain.extra_binaries,
         src_root = src_root,
         env = env,
         mode = ctx.var["COMPILATION_MODE"],

haskell/private/haddock_wrapper.sh → haskell/private/haddock_wrapper.sh.tpl

@@ -4,6 +4,8 @@
 
 set -eo pipefail
 
+%{env}
+
 PREBUILT_DEPS_FILE=$1
 shift
 
@@ -19,8 +21,8 @@ do
     # If there were more than one file, going by the output for the `depends`,
     # the file names would be separated by a space character.
     # https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/packages.html#installedpackageinfo-a-package-specification
-    haddock_interfaces=$(ghc-pkg --simple-output field $pkg haddock-interfaces)
-    haddock_html=$(ghc-pkg --simple-output field $pkg haddock-html)
+    haddock_interfaces=$(%{ghc-pkg} --simple-output field $pkg haddock-interfaces)
+    haddock_html=$(%{ghc-pkg} --simple-output field $pkg haddock-html)
 
     # Sometimes the referenced `.haddock` file does not exist
     # (e.g. for `nixpkgs.haskellPackages` deps with haddock disabled).
@@ -43,5 +45,5 @@ cleanup() { rmdir "$TEMP"; }
 # XXX Override TMPDIR to prevent race conditions on certain platforms.
 # This is a workaround for
 # https://github.com/haskell/haddock/issues/894.
-TMPDIR=$TEMP haddock "${extra_args[@]}" "$@"
+TMPDIR=$TEMP %{haddock} "${extra_args[@]}" "$@"
 cleanup