From b06c91577b5e11905e3b2e961a525d67ad5e2bcf Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Mon, 16 Mar 2020 12:07:43 +0100 Subject: [PATCH 1/2] RUNPATH and runfiles for transitive C libs GHC passes `-l` flags for all transitive C library dependencies when linking. Accordingly, we need to provide `RUNPATH` entries for all transitive C libraries. Also, to ensure that binaries can find their dynamic library dependencies, we add them to the `runfiles` attribute of the `DefaultInfo` provider. --- haskell/cabal.bzl | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/haskell/cabal.bzl b/haskell/cabal.bzl index ff0845994..4ae3dfc80 100644 --- a/haskell/cabal.bzl +++ b/haskell/cabal.bzl @@ -32,7 +32,9 @@ load( load( ":private/cc_libraries.bzl", "deps_HaskellCcLibrariesInfo", + "get_cc_libraries", "get_ghci_library_files", + "get_library_files", "haskell_cc_libraries_aspect", ) @@ -110,6 +112,9 @@ def _cabal_tool_flag(tool): def _binary_paths(binaries): return [binary.dirname for binary in binaries.to_list()] +def _concat(sequences): + return [item for sequence in sequences for item in sequence] + def _prepare_cabal_inputs( hs, cc, @@ -139,7 +144,26 @@ def _prepare_cabal_inputs( # to add libraries and headers for direct C library dependencies to the # command line. direct_libs = get_ghci_library_files(hs, cc.cc_libraries_info, cc.cc_libraries) - transitive_libs = get_ghci_library_files(hs, cc.cc_libraries_info, cc.transitive_libraries) + + # The regular Haskell rules perform mostly static linking, i.e. where + # possible all C library dependencies are linked statically. Cabal has no + # such mode, and since we have to provide dynamic C libraries for + # compilation, they will also be used for linking. Hence, we need to add + # RUNPATH flags for all dynamic C library dependencies. + (_, dynamic_libs) = get_library_files( + hs, + cc.cc_libraries_info, + get_cc_libraries(cc.cc_libraries_info, cc.transitive_libraries), + dynamic = True, + ) + + # The regular Haskell rules have separate actions for linking and + # compilation to which we pass different sets of libraries as inputs. The + # Cabal rules, in contrast, only have a single action for compilation and + # linking, so we must provide both sets of libraries as inputs to the same + # action. + transitive_compile_libs = get_ghci_library_files(hs, cc.cc_libraries_info, cc.transitive_libraries) + transitive_link_libs = _concat(get_library_files(hs, cc.cc_libraries_info, cc.transitive_libraries)) env = dict(hs.env) env["PATH"] = join_path_list(hs, _binary_paths(tool_inputs) + posix.paths) if hs.toolchain.is_darwin: @@ -169,7 +193,7 @@ def _prepare_cabal_inputs( keep_filename = False, prefix = relative_rpath_prefix(hs.toolchain.is_darwin), ) - for lib in direct_libs + for lib in dynamic_libs ], uniquify = True, ) @@ -190,7 +214,8 @@ def _prepare_cabal_inputs( depset(cc.files), package_databases, transitive_headers, - depset(transitive_libs), + depset(transitive_compile_libs), + depset(transitive_link_libs), dep_info.interface_dirs, dep_info.static_libraries, dep_info.dynamic_libraries, @@ -205,6 +230,7 @@ def _prepare_cabal_inputs( inputs = inputs, input_manifests = input_manifests, env = env, + runfiles = depset(direct = dynamic_libs), ) def _haskell_cabal_library_impl(ctx): @@ -572,6 +598,7 @@ def _haskell_cabal_binary_impl(ctx): executable = binary, runfiles = ctx.runfiles( files = [data_dir], + transitive_files = c.runfiles, collect_default = True, ), ) From 8a88b9fef19be4a1eaa153eea6fef7c762867c6e Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Mon, 16 Mar 2020 12:36:44 +0100 Subject: [PATCH 2/2] Add Cabal binary RUNPATH test case Regression test for Cabal binary targets not including RUNPATH entries for transitive C library dependencies. --- tests/stackage_zlib_runpath/BUILD.bazel | 138 +++++++++++------- .../cabal-binary/Main.hs | 4 + .../cabal-binary/cabal-binary.cabal | 11 ++ 3 files changed, 101 insertions(+), 52 deletions(-) create mode 100644 tests/stackage_zlib_runpath/cabal-binary/Main.hs create mode 100644 tests/stackage_zlib_runpath/cabal-binary/cabal-binary.cabal diff --git a/tests/stackage_zlib_runpath/BUILD.bazel b/tests/stackage_zlib_runpath/BUILD.bazel index 7826d77d9..dd3ec3e11 100644 --- a/tests/stackage_zlib_runpath/BUILD.bazel +++ b/tests/stackage_zlib_runpath/BUILD.bazel @@ -1,3 +1,7 @@ +load( + "@rules_haskell//haskell:cabal.bzl", + "haskell_cabal_binary", +) load( "//tests:inline_tests.bzl", "py_inline_test", @@ -19,12 +23,23 @@ dynamic_libraries( tags = ["requires_zlib"], ) -# Tests that haskell_cabal_library will generate a relative RUNPATH entry for -# the dependency on the nixpkgs provided libz. Relative meaning an entry that -# starts with $ORIGIN (Linux) or @loader_path (MacOS). The alternative is an -# absolute path, which would be wrong for the nixpkgs provided libz, as we want -# the RUNPATH entry to point to Bazel's _solib_ directory and its absolute -# path depends on the output root or execroot. +haskell_cabal_binary( + name = "cabal-binary", + srcs = glob(["cabal-binary/**"]), + tags = ["requires_zlib"], + deps = [ + "//tests/hackage:base", + # Depend transitively on libz. + "@stackage-zlib//:zlib", + ], +) + +# Tests that haskell_cabal_library|binary will generate a relative RUNPATH +# entry for the dependency on the nixpkgs provided libz. Relative meaning an +# entry that starts with $ORIGIN (Linux) or @loader_path (MacOS). The +# alternative is an absolute path, which would be wrong for the nixpkgs +# provided libz, as we want the RUNPATH entry to point to Bazel's _solib_ +# directory and its absolute path depends on the output root or execroot. # # It uses :libz_soname generated above to determine the expected RUNPATH entry # for the libz dependency. The :libz_soname file will contain the file names of @@ -33,17 +48,22 @@ dynamic_libraries( # It uses :libHSzlib to access the dynamic library output of # haskell_cabal_library and read the RUNPATH entries. # -# Note, ideally we would test that haskell_cabal_library _only_ generates a -# relative RUNPATH entry and no absolute entries that leak the execroot into -# the cache. Unfortunately, haskell_cabal_library generates such an entry at -# the moment. See https://github.com/tweag/rules_haskell/issues/1130. +# It uses :cabal-binary to access a binary that transitively depends on libz. +# +# Note, ideally we would test that haskell_cabal_library|binary _only_ +# generates a relative RUNPATH entry and no absolute entries that leak the +# execroot into the cache. Unfortunately, haskell_cabal_library|binary +# generates such an entry at the moment. See +# https://github.com/tweag/rules_haskell/issues/1130. py_inline_test( name = "stackage_zlib_runpath", args = [ "$(rootpath :libz_soname)", "$(rootpath :libHSzlib)", + "$(rootpath :cabal-binary)", ], data = [ + ":cabal-binary", ":libHSzlib", ":libz_soname", ], @@ -65,57 +85,71 @@ with open(libz_soname) as fh: sofile = fh.read().splitlines()[1] sodir = os.path.dirname(sofile) -# Determine libHSzlib RUNPATH +# Locate test artifacts. libHSzlib = r.Rlocation(os.path.join( os.environ["TEST_WORKSPACE"], sys.argv[2], )) -runpaths = [] -if platform.system() == "Darwin": - dynamic_section = iter(subprocess.check_output(["otool", "-l", libHSzlib]).decode().splitlines()) - # otool produces lines of the form - # - # Load command ... - # cmd LC_RPATH - # cmdsize ... - # path ... - # - for line in dynamic_section: - # Find LC_RPATH entry - if line.find("cmd LC_RPATH") != -1: - break - # Skip until path field +cabal_binary = r.Rlocation(os.path.join( + os.environ["TEST_WORKSPACE"], + sys.argv[3], +)) + +def read_runpaths(binary): + runpaths = [] + if platform.system() == "Darwin": + dynamic_section = iter(subprocess.check_output(["otool", "-l", binary]).decode().splitlines()) + # otool produces lines of the form + # + # Load command ... + # cmd LC_RPATH + # cmdsize ... + # path ... + # for line in dynamic_section: - if line.strip().startswith("path"): + # Find LC_RPATH entry + if line.find("cmd LC_RPATH") != -1: break - runpaths.append(line.split()[1]) -else: - dynamic_section = subprocess.check_output(["objdump", "--private-headers", libHSzlib]).decode().splitlines() - # objdump produces lines of the form - # - # Dynamic Section: - # ... - # RUNPATH ... - # ... - for line in dynamic_section: - if not line.strip().startswith("RUNPATH"): + # Skip until path field + for line in dynamic_section: + if line.strip().startswith("path"): + break + runpaths.append(line.split()[1]) + else: + dynamic_section = subprocess.check_output(["objdump", "--private-headers", binary]).decode().splitlines() + # objdump produces lines of the form + # + # Dynamic Section: + # ... + # RUNPATH ... + # ... + for line in dynamic_section: + if not line.strip().startswith("RUNPATH"): + continue + runpaths.extend(line.split()[1].split(":")) + + return runpaths + +def test_binary(binary, sodir): + runpaths = read_runpaths(binary) + # Check that the binary contains a relative RUNPATH for sodir. + found = False + for runpath in runpaths: + if runpath.find(sodir) == -1: continue - runpaths.extend(line.split()[1].split(":")) + if runpath.startswith("$ORIGIN") or runpath.startswith("@loader_path"): + found = True + # XXX: Enable once #1130 is fixed. + #if os.path.isabs(runpath): + # print("Absolute RUNPATH entry discovered for %s: %s" % (sodir, runpath)) + # sys.exit(1) + + if not found: + print("Did not find a relative RUNPATH entry for %s among %s." % (sodir, runpaths)) -# Check that the binary contains a relative RUNPATH for sodir. -found = False -for runpath in runpaths: - if runpath.find(sodir) == -1: - continue - if runpath.startswith("$ORIGIN") or runpath.startswith("@loader_path"): - found = True - # XXX: Enable once #1130 is fixed. - #if os.path.isabs(runpath): - # print("Absolute RUNPATH entry discovered for %s: %s" % (sodir, runpath)) - # sys.exit(1) + return found -if not found: - print("Did not find a relative RUNPATH entry for %s among %s." % (sodir, runpaths)) +if not all(test_binary(binary, sodir) for binary in [libHSzlib, cabal_binary]): sys.exit(1) """, tags = ["requires_zlib"], diff --git a/tests/stackage_zlib_runpath/cabal-binary/Main.hs b/tests/stackage_zlib_runpath/cabal-binary/Main.hs new file mode 100644 index 000000000..89ad4b3e0 --- /dev/null +++ b/tests/stackage_zlib_runpath/cabal-binary/Main.hs @@ -0,0 +1,4 @@ +module Main where + +main :: IO () +main = pure () diff --git a/tests/stackage_zlib_runpath/cabal-binary/cabal-binary.cabal b/tests/stackage_zlib_runpath/cabal-binary/cabal-binary.cabal new file mode 100644 index 000000000..4b31a4114 --- /dev/null +++ b/tests/stackage_zlib_runpath/cabal-binary/cabal-binary.cabal @@ -0,0 +1,11 @@ +cabal-version: >=1.10 +name: cabal-binary +version: 0.1.0.0 +build-type: Simple + +executable cabal-binary + build-depends: + base, + zlib + default-language: Haskell2010 + main-is: Main.hs