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.
tweag · Jan 13, 2019 · 7f16f6e · 7f16f6e
1 parent ffa972f
commit 7f16f6e
Show file tree

Hide file tree

Showing 14 changed files with 126 additions and 233 deletions.
diff --git a/CHANGELOG.md b/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
 

diff --git a/haskell/BUILD b/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",
     ],
 )
 

diff --git a/haskell/c2hs.bzl b/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,22 @@ 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),
+            depset(cc.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 +92,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"),
         ),

diff --git a/haskell/cc.bzl b/haskell/cc.bzl
@@ -22,6 +22,10 @@ load(
 CcInteropInfo = provider(
     doc = "Information needed for interop with cc rules.",
     fields = {
+        "tools": "Tools from the CC toolchain",
+        # See the following for why this is needed:
+        # https://stackoverflow.com/questions/52769846/custom-c-rule-with-the-cc-common-api
+        "files": "Files for all tools (input to any action that uses tools)",
         "hdrs": "CC headers",
         "cpp_flags": "Preprocessor flags",
         "compiler_flags": "Flags for compilation",
@@ -121,7 +125,24 @@ 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),
+        files = ctx.files._cc_toolchain,
         hdrs = hdrs.to_list(),
         cpp_flags = cpp_flags,
         include_args = include_args,

diff --git a/haskell/doctest.bzl b/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

diff --git a/haskell/haddock.bzl b/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}"

diff --git a/haskell/lint.bzl b/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))

diff --git a/haskell/private/actions/compile.bzl b/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,8 @@ 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]),
+            depset(cc.files),
         ]),
         outputs = [hs_out],
         mnemonic = "HaskellHsc2hs",
@@ -250,7 +251,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 +291,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 +349,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),

diff --git a/haskell/private/actions/link.bzl b/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] + cc.files
 
     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),
             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),

diff --git a/haskell/private/c2hs_wrapper.sh b/haskell/private/c2hs_wrapper.sh
diff --git a/haskell/private/context.bzl b/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"],

diff --git a/haskell/private/haddock_wrapper.sh → haskell/private/haddock_wrapper.sh.tpl b/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