From 5164550b2fc1628fb0a3c70b6618eb7fb608fac9 Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Thu, 5 Mar 2020 10:25:41 +0100 Subject: [PATCH 1/5] Pass -rpath flags to Cabal By default Cabal generates `RUNPATH` entries pointing to the sandbox directory. These no longer work outside of that particular sandbox. --- haskell/cabal.bzl | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/haskell/cabal.bzl b/haskell/cabal.bzl index e94942382..effa5e6f4 100644 --- a/haskell/cabal.bzl +++ b/haskell/cabal.bzl @@ -9,7 +9,12 @@ load(":private/context.bzl", "haskell_context", "render_env") load(":private/dependencies.bzl", "gather_dep_info") load(":private/expansions.bzl", "expand_make_variables") load(":private/mode.bzl", "is_profiling_enabled") -load(":private/path_utils.bzl", "join_path_list", "truly_relativize") +load( + ":private/path_utils.bzl", + "create_rpath_entry", + "join_path_list", + "truly_relativize", +) load(":private/set.bzl", "set") load(":haddock.bzl", "generate_unified_haddock_info") load( @@ -103,7 +108,7 @@ def _cabal_tool_flag(tool): def _binary_paths(binaries): return [binary.dirname for binary in binaries.to_list()] -def _prepare_cabal_inputs(hs, cc, posix, dep_info, cc_info, direct_cc_info, component, package_id, tool_inputs, tool_input_manifests, cabal, setup, srcs, compiler_flags, flags, generate_haddock, cabal_wrapper, package_database, verbose): +def _prepare_cabal_inputs(hs, cc, posix, dep_info, cc_info, direct_cc_info, component, package_id, tool_inputs, tool_input_manifests, cabal, setup, srcs, compiler_flags, flags, generate_haddock, cabal_wrapper, package_database, verbose, dynamic_binary = None): """Compute Cabal wrapper, arguments, inputs.""" with_profiling = is_profiling_enabled(hs) @@ -133,6 +138,19 @@ def _prepare_cabal_inputs(hs, cc, posix, dep_info, cc_info, direct_cc_info, comp args.add_all([component, package_id, generate_haddock, setup, cabal.dirname, package_database.dirname]) args.add("--flags=" + " ".join(flags)) args.add_all(compiler_flags, format_each = "--ghc-option=%s") + if dynamic_binary: + args.add_all( + [ + "--ghc-option=-optl-Wl,-rpath," + create_rpath_entry( + binary = dynamic_binary, + dependency = lib, + keep_filename = False, + prefix = "@loader_path" if hs.toolchain.is_darwin else "$ORIGIN", + ) + for lib in direct_libs + ], + uniquify = True, + ) args.add("--") args.add_all(package_databases, map_each = _dirname, format_each = "--package-db=%s") args.add_all(direct_include_dirs, format_each = "--extra-include-dirs=%s") @@ -260,6 +278,7 @@ def _haskell_cabal_library_impl(ctx): cabal_wrapper = ctx.executable._cabal_wrapper, package_database = package_database, verbose = ctx.attr.verbose, + dynamic_binary = dynamic_library, ) outputs = [ package_database, @@ -497,6 +516,7 @@ def _haskell_cabal_binary_impl(ctx): cabal_wrapper = ctx.executable._cabal_wrapper, package_database = package_database, verbose = ctx.attr.verbose, + dynamic_binary = binary, ) ctx.actions.run( executable = c.cabal_wrapper, From 8f4ed5707e735baa221196fd1a454995645d94de Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Thu, 5 Mar 2020 16:29:49 +0100 Subject: [PATCH 2/5] Add a regression test Adds a regression test to check that haskell_cabal_library will generate a relative `RUNPATH` entry for the nixpkgs provided zlib. --- tests/stackage_zlib_runpath/BUILD.bazel | 122 ++++++++++++++++++ .../dynamic_libraries.bzl | 38 ++++++ 2 files changed, 160 insertions(+) create mode 100644 tests/stackage_zlib_runpath/BUILD.bazel create mode 100644 tests/stackage_zlib_runpath/dynamic_libraries.bzl diff --git a/tests/stackage_zlib_runpath/BUILD.bazel b/tests/stackage_zlib_runpath/BUILD.bazel new file mode 100644 index 000000000..87652342f --- /dev/null +++ b/tests/stackage_zlib_runpath/BUILD.bazel @@ -0,0 +1,122 @@ +load( + "//tests:inline_tests.bzl", + "py_inline_test", +) +load("dynamic_libraries.bzl", "dynamic_libraries") + +dynamic_libraries( + name = "libz", + srcs = ["@zlib.dev//:zlib"], + filter = "libz", + solib_names = "libz_soname", + tags = ["requires_zlib"], +) + +dynamic_libraries( + name = "libHSzlib", + srcs = ["@stackage-zlib//:zlib"], + filter = "libHSz", + tags = ["requires_zlib"], +) + +# This test case 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. +# +# 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 +# the libz library files underneath the `_solib_` directory. +# +# 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. +py_inline_test( + name = "stackage_zlib_runpath", + args = [ + "$(rootpath :libz_soname)", + "$(rootpath :libHSzlib)", + ], + data = [ + ":libHSzlib", + ":libz_soname", + ], + script = """\ +from bazel_tools.tools.python.runfiles import runfiles as bazel_runfiles +import itertools +import os +import platform +import subprocess +import sys +r = bazel_runfiles.Create() + +# Determine libz solib directory +libz_soname = r.Rlocation(os.path.join( + os.environ["TEST_WORKSPACE"], + sys.argv[1], +)) +with open(libz_soname) as fh: + sofile = fh.read().splitlines()[1] + sodir = os.path.dirname(sofile) + +# Determine libHSzlib RUNPATH +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 + 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", libHSzlib]).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(":")) + +# 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) + +if not found: + print("Did not find a relative RUNPATH entry for %s among %s." % (sodir, runpaths)) + sys.exit(1) +""", + tags = ["requires_zlib"], +) diff --git a/tests/stackage_zlib_runpath/dynamic_libraries.bzl b/tests/stackage_zlib_runpath/dynamic_libraries.bzl new file mode 100644 index 000000000..78f55bce3 --- /dev/null +++ b/tests/stackage_zlib_runpath/dynamic_libraries.bzl @@ -0,0 +1,38 @@ +def _dynamic_libraries_impl(ctx): + outputs = [] + solib_names = [] + for target in ctx.attr.srcs: + cc_info = target[CcInfo] + for library_to_link in cc_info.linking_context.libraries_to_link.to_list(): + library = library_to_link.resolved_symlink_dynamic_library + if not library or library.basename.find(ctx.attr.filter) == -1: + continue + outputs.append(library) + if library_to_link.dynamic_library: + solib_names.append(library_to_link.dynamic_library.short_path) + if ctx.attr.solib_names: + ctx.actions.write( + ctx.outputs.solib_names, + "\n".join(solib_names), + ) + return [DefaultInfo( + files = depset(outputs), + runfiles = ctx.runfiles(files = outputs), + )] + +dynamic_libraries = rule( + _dynamic_libraries_impl, + attrs = { + "filter": attr.string( + doc = "Skip libraries that do not contain this string in their name.", + ), + "srcs": attr.label_list( + doc = "Extract dynamic libraries from these targets", + providers = [CcInfo], + ), + "solib_names": attr.output( + doc = "Write the `_solib_` paths of the dynamic libraries to this file.", + ), + }, + doc = "Extract the dynamic libraries from cc_library targets.", +) From 380880d1b69b5c2d5b963ea70e5c9489c0bd4bb6 Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Fri, 6 Mar 2020 10:18:03 +0100 Subject: [PATCH 3/5] Line break _prepare_cabal_inputs parameters. Addressing https://github.com/tweag/rules_haskell/pull/1267#discussion_r388743621 --- haskell/cabal.bzl | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/haskell/cabal.bzl b/haskell/cabal.bzl index effa5e6f4..12a6136f0 100644 --- a/haskell/cabal.bzl +++ b/haskell/cabal.bzl @@ -108,7 +108,27 @@ def _cabal_tool_flag(tool): def _binary_paths(binaries): return [binary.dirname for binary in binaries.to_list()] -def _prepare_cabal_inputs(hs, cc, posix, dep_info, cc_info, direct_cc_info, component, package_id, tool_inputs, tool_input_manifests, cabal, setup, srcs, compiler_flags, flags, generate_haddock, cabal_wrapper, package_database, verbose, dynamic_binary = None): +def _prepare_cabal_inputs( + hs, + cc, + posix, + dep_info, + cc_info, + direct_cc_info, + component, + package_id, + tool_inputs, + tool_input_manifests, + cabal, + setup, + srcs, + compiler_flags, + flags, + generate_haddock, + cabal_wrapper, + package_database, + verbose, + dynamic_binary = None): """Compute Cabal wrapper, arguments, inputs.""" with_profiling = is_profiling_enabled(hs) From 61792e3ae1753a90ee09dc8df1cd968d9afcbec0 Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Fri, 6 Mar 2020 10:24:20 +0100 Subject: [PATCH 4/5] Factor out relative RUNPATH prefix Addressing https://github.com/tweag/rules_haskell/pull/1267#discussion_r388743958 --- haskell/cabal.bzl | 3 ++- haskell/private/cc_libraries.bzl | 3 ++- haskell/private/path_utils.bzl | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/haskell/cabal.bzl b/haskell/cabal.bzl index 12a6136f0..79b88a8b8 100644 --- a/haskell/cabal.bzl +++ b/haskell/cabal.bzl @@ -13,6 +13,7 @@ load( ":private/path_utils.bzl", "create_rpath_entry", "join_path_list", + "relative_rpath_prefix", "truly_relativize", ) load(":private/set.bzl", "set") @@ -165,7 +166,7 @@ def _prepare_cabal_inputs( binary = dynamic_binary, dependency = lib, keep_filename = False, - prefix = "@loader_path" if hs.toolchain.is_darwin else "$ORIGIN", + prefix = relative_rpath_prefix(hs.toolchain.is_darwin), ) for lib in direct_libs ], diff --git a/haskell/private/cc_libraries.bzl b/haskell/private/cc_libraries.bzl index 805e30b50..d8b33532d 100644 --- a/haskell/private/cc_libraries.bzl +++ b/haskell/private/cc_libraries.bzl @@ -18,6 +18,7 @@ load( "get_lib_name", "mangle_static_library", "rel_to_pkgroot", + "relative_rpath_prefix", "target_unique_name", ) load( @@ -211,7 +212,7 @@ def create_link_config(hs, posix, cc_libraries_info, libraries_to_link, binary, binary = binary, dependency = lib, keep_filename = False, - prefix = "@loader_path" if hs.toolchain.is_darwin else "$ORIGIN", + prefix = relative_rpath_prefix(hs.toolchain.is_darwin), ) for lib in dynamic_libs ]), diff --git a/haskell/private/path_utils.bzl b/haskell/private/path_utils.bzl index 0ddf6b460..edfd7bcf8 100644 --- a/haskell/private/path_utils.bzl +++ b/haskell/private/path_utils.bzl @@ -275,6 +275,20 @@ def _get_target_parent_dir(target): __check_dots(target, parent_dir) return (False, parent_dir) +def relative_rpath_prefix(is_darwin): + """Returns the prefix for relative RUNPATH entries. + + Args: + is_darwin: Whether the target platform is Darwin. + + Returns: + string, `@loader_path` on Darwin, `$ORIGIN` otherwise. + """ + if is_darwin: + return "@loader_path" + else: + return "$ORIGIN" + # tests in /tests/unit_tests/BUILD def create_rpath_entry( binary, From 0a1b977578695906406b823edbf378392e094157 Mon Sep 17 00:00:00 2001 From: Andreas Herrmann Date: Fri, 6 Mar 2020 10:25:33 +0100 Subject: [PATCH 5/5] Simplify test-case comment Addressing https://github.com/tweag/rules_haskell/pull/1267#discussion_r388771443 --- tests/stackage_zlib_runpath/BUILD.bazel | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/stackage_zlib_runpath/BUILD.bazel b/tests/stackage_zlib_runpath/BUILD.bazel index 87652342f..7826d77d9 100644 --- a/tests/stackage_zlib_runpath/BUILD.bazel +++ b/tests/stackage_zlib_runpath/BUILD.bazel @@ -19,12 +19,12 @@ dynamic_libraries( tags = ["requires_zlib"], ) -# This test case 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. +# 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. # # 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