Use relative path in installed libraries

[fbudin69500]

On MacOS, libraries can be modified at installation to update the path to shared libraries used using the tool install_name_tool. On linux, there is no easy way to fixup the paths to the libraries that are used (patchelf can only update the library paths in its most recent version that is not widely available). Instead, on Linux, the relative path in which libraries should be found at runtime is added to RPath. This patch allows to not manually have to set (DY)LD_LIBRARY_PATH when using pydrake: All the dependencies will be found automatically.

---

This change is 

[jwnimmer-tri]

On linux, there is no easy way to fixup the paths to the libraries that are used ...

As it happens, today I've been testing out /usr/bin/chrpath to change .so RPATHs. Does it not work for this PR's purpose?

[fbudin69500]

chrpath on linux could work. It would add a dependency, and the logic that has been added to pybind.bzl would move to install.py.in(it would be similar to what is done for MacOS). Both approaches should give the exact same result. It would not allow to change the individual path of each library like install_name_tool or patchelf would.

---

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

drake/bindings/pybind.bzl, line 27 at r1 (raw file):

    # Count folders in name to find relative path to lib subfolder.
    relative_path = '/..' * (name.count('/') + 1)

It is unclear why the count of two slashes found in "python/pydrake/_pydrake_common.so" are directly related to the number of .. steps we need to go from, e.g., "lib/python2.7/site-packages/pydrake/_pydrake_symbolic.so" to "lib/libdrake.so".

Probably the way to make it clear would be to assert that name.startswith("python/"), then count that as two (explaining "python#.#/site-packages"), and then take any additional module paths (one, in most cases) and count those as well.

---

tools/install.py.in, line 101 at r1 (raw file):

                 dst_full]
                )
        # Remove rpath values that contain $ORIGIN. These are from the build

The fix_library_paths is only run in the case of sys.platform == "darwin". The changes in pybind.bzl only add "-Wl,-rpath,$$ORIGIN" ... when not "//tools/cc_toolchain:apple". Thus, I don't understand where $ORIGIN is coming from. Is Bazel itself adding that rpath, outside of our skylark code?

---

tools/install.py.in, line 106 at r1 (raw file):

        for line in file_output.splitlines():
            split_line = line.strip().split(' ')
            if len(split_line) >= 2 and 'path' == split_line[0] and split_line[1].startswith('$ORIGIN'):

line too long

---

tools/install.py.in, line 107 at r1 (raw file):

            split_line = line.strip().split(' ')
            if len(split_line) >= 2 and 'path' == split_line[0] and split_line[1].startswith('$ORIGIN'):
                # path is specified 2 lines below

I don't understand what this comment means.

---

Comments from Reviewable

[jwnimmer-tri]

drake/bindings/pybind.bzl, line 26 at r1 (raw file):

            fail("%s cannot be set by the caller" % key)

    # Count folders in name to find relative path to lib subfolder.

To avoid confusing a reader of the code here, this documentation here should make it clear that we are performing an installation-related action at build-time instead of install-time, because we do not want to depend on chrpath during install. In other words, make it clear that this rpath is not relevant for the sandbox nor for building nor testing, but rather only once this *.so file has been copied into an install tree.

---

Comments from Reviewable

[jamiesnape]

Marking high priority since this is required for one of the tests in #7293 to pass.

[fbudin69500]

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

tools/install.py.in, line 101 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The fix_library_paths is only run in the case of sys.platform == "darwin". The changes in pybind.bzl only add "-Wl,-rpath,$$ORIGIN" ... when not "//tools/cc_toolchain:apple". Thus, I don't understand where $ORIGIN is coming from. Is Bazel itself adding that rpath, outside of our skylark code?

To answer the question, the linker adds values to RPATH to find the libraries in the build tree. This is done both on linux and on MacOS, and not limited to bazel. On MacOS, this can create the SIP error reported by @ EricCousineau-TRI in #6760 . Since the paths added to RPATH at build time are irrelevant at install time, it is better to remove them.
However, in this new version of the PR, the fix is now done at install time for both MacOS and Linux. This is because the previous version of the patch was only addressing the issue for the pydrake libraries, whereas this new patch should solve the problem for all the libraries (one limitation is that the libraries must already contain an RPATH, otherwise they cannot be fixed at install time, e.g. VTK libraries).

---

tools/install.py.in, line 106 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

line too long

Done.

---

tools/install.py.in, line 107 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't understand what this comment means.

Removed.

---

drake/bindings/pybind.bzl, line 26 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To avoid confusing a reader of the code here, this documentation here should make it clear that we are performing an installation-related action at build-time instead of install-time, because we do not want to depend on chrpath during install. In other words, make it clear that this rpath is not relevant for the sandbox nor for building nor testing, but rather only once this *.so file has been copied into an install tree.

This new version of the patch actually removes the modifications of pybind.bzl and uses chrpath to update RPATH on linux at install time. This is necessary to fix all the libraries and not only pydrake libraries.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 3 files at r2.
Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[fbudin69500]

One problem that this patch does not address is the RPATH of the executables, but that doesn't seem to be needed for now.

---

Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[jwnimmer-tri]

FYI I am OK with the chrpath addition to the prereqs, if someone wants to get those new AMIs underway.

[jwnimmer-tri]

PR needs a rebase onto recent master.

---

Reviewed 2 of 3 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
We're past the point where the amount of code in install.py.in is consistent with a codegen template (a *.in file). We're missing out on pycodestyle checking of it, as well as the easy ability to unit test the intricate new (and existing) functions.

Ideally, all of the library fixup methods would have standalone branch-coverage level unit testing with python's unittest library that include both common and corner cases. It's great that we have testCommonInstall.py as an acceptance test, but focused unit testing would enable reviewers to rely on CI, instead of having to put the code through its paces locally, or rely on out-of-tree CI to test it.

Those function need to turn into *.py code that is imported by the generated :install program(s). Since this PR is high priority, I would be willing to accept a high-priority issue number that will be fixed immediately after this PR merges, instead of blocking this PR on it.

---

tools/install.py.in, line 68 at r1 (raw file):

        libs[basename] = (dst_full, installed)

BTW Needs two lines between function declarations per pycodestyle.

---

tools/install.py.in, line 17 at r2 (raw file):

# On linux, dynamic libraries may have their version number
# as a suffix (e.g. my_lib.so.x.y.z).
dylib_match = r'(.*.so)(\.\d+)*$|(.*dylib)$'

BTW Do you want to escape the . in .so to force it to literally match a period?

---

tools/install.py.in, line 60 at r2 (raw file):

        print("[Up to date] %s" % dst)
    m = re.match(dylib_match , dst)
    if m != None:

FYI Python styleguides usually say m is not None as the required spelling.

---

tools/install.py.in, line 85 at r2 (raw file):

BTW nit Three blank lines? Should be two, I think.

---

tools/install.py.in, line 122 at r2 (raw file):

            ['install_name_tool', "-delete_rpath", split_line[1], dst_full]
            )

BTW Needs two lines between function declarations per pycodestyle.

---

tools/install.py.in, line 126 at r2 (raw file):

    # Check that library has an rpath or a runpath tag
    try:
      check_output(["chrpath", "-l", dst_full])

Bad indentation. Run a Python style checker, I guess manually for now.

---

tools/install.py.in, line 126 at r2 (raw file):

    # Check that library has an rpath or a runpath tag
    try:
      check_output(["chrpath", "-l", dst_full])

FYI Consider printing an informative error message if chrpath program is not present. (Since it's a new dependency, some users are likely to forget to install it.)

---

tools/install.py.in, line 129 at r2 (raw file):

    except CalledProcessError as ex:
      if ex.output.strip().endswith('no rpath or runpath tag found.'):
        # Cannot be modified with `chrpath`, so we skip it.

If the no rpath message is not present in the output we ... still ignore the error? Shouldn't we re-raise it instead, or print and exit, or ...?

---

tools/install.py.in, line 139 at r2 (raw file):

        if len(ldd_result) < 2 or ldd_result[1] != "not found":
            continue
        m = re.match(dylib_match, ldd_result[0])

Python best practice would be to to a structured assignment, something like:

soname, sover = m.groups()

In this way, if the number of groups in the regex ever evolves, we would fail-fast instead of computing with lies.

---

tools/install.py.in, line 148 at r2 (raw file):

          os.path.relpath(libs[m.group(1)][0], lib_dirname)
        )
        if diff_path:

Unclear when this is ever false? Should the / just appear in the subsequent line, instead?

---

Comments from Reviewable

[fbudin69500]

Review status: 0 of 3 files reviewed at latest revision, 11 unresolved discussions.

---

tools/install.py.in, line 68 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Needs two lines between function declarations per pycodestyle.

Done.

---

tools/install.py.in, line 17 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Do you want to escape the . in .so to force it to literally match a period?

Good idea, and same with dylib.

---

tools/install.py.in, line 60 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Python styleguides usually say m is not None as the required spelling.

Done.

---

tools/install.py.in, line 85 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW nit Three blank lines? Should be two, I think.

Done.

---

tools/install.py.in, line 122 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Needs two lines between function declarations per pycodestyle.

Done.

---

tools/install.py.in, line 126 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Bad indentation. Run a Python style checker, I guess manually for now.

Done.

---

tools/install.py.in, line 126 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Consider printing an informative error message if chrpath program is not present. (Since it's a new dependency, some users are likely to forget to install it.)

Done.

---

tools/install.py.in, line 129 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If the no rpath message is not present in the output we ... still ignore the error? Shouldn't we re-raise it instead, or print and exit, or ...?

Done.

---

tools/install.py.in, line 139 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Python best practice would be to to a structured assignment, something like:

soname, sover = m.groups()

In this way, if the number of groups in the regex ever evolves, we would fail-fast instead of computing with lies.

Done.

---

tools/install.py.in, line 148 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear when this is ever false? Should the / just appear in the subsequent line, instead?

It was to avoid having a trailing / in RPATH, butit does not matter.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 2 of 4 files at r3.
Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

tools/install/install.py.in, line 138 at r3 (raw file):

        if ex.errno == 2 and ex.strerror == "No such file or directory":
            print("`chrpath` not found. Please run install_prereqs.sh.")
            sys.exit(1)

The not-if condition should re-raise, I think?

    except OSError as ex:
        if ex.errno == 2 and ex.strerror == "No such file or directory":
            print("`chrpath` not found. Please run install_prereqs.sh.")
        raise ex
    except ...

---

tools/install/install.py.in, line 144 at r3 (raw file):

                # Cannot be modified with `chrpath`, so we skip it.
                return
        else:

When returncode is 2, but the output didn't match, we sliently ignore the error.

Probably the else should disappear, and the re-raise should de-indent four spaces?

---

tools/install.py.in, line 148 at r2 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

It was to avoid having a trailing / in RPATH, butit does not matter.

OK But I didn't think diff_path would ever be empty? It would be ., at worst.

---

Comments from Reviewable

[fbudin69500]

Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

tools/install/install.py.in, line 138 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The not-if condition should re-raise, I think?

    except OSError as ex:
        if ex.errno == 2 and ex.strerror == "No such file or directory":
            print("`chrpath` not found. Please run install_prereqs.sh.")
        raise ex
    except ...

Done.

---

tools/install/install.py.in, line 144 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

When returncode is 2, but the output didn't match, we sliently ignore the error.

Probably the else should disappear, and the re-raise should de-indent four spaces?

Done.

---

tools/install.py.in, line 148 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK But I didn't think diff_path would ever be empty? It would be ., at worst.

Actually it does return an empty string. os.path.relpath() returns . but os.path.dirname() returns an empty string when called on .

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 4 files at r3.
Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

tools/install/install.py.in, line 138 at r3 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

Done.

I think the re-raise should be out one level of indentation? So that it always happens?

(This is why PRs without unit tests are a challenge, for both author and reviewer.)

---

tools/install/install.py.in, line 143 at r4 (raw file):

            and ex.output.strip().endswith('no rpath or runpath tag found.'):
                # Cannot be modified with `chrpath`, so we skip it.
                return

Wrong level of intendation -- should be only +4 versus the if this body is filling in.

---

tools/install.py.in, line 148 at r2 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

Actually it does return an empty string. os.path.relpath() returns . but os.path.dirname() returns an empty string when called on .

OK I see now. If you wanted to put it back with a comment, I'd be OK with that too.

---

Comments from Reviewable

[fbudin69500]

Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

tools/install/install.py.in, line 138 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think the re-raise should be out one level of indentation? So that it always happens?

(This is why PRs without unit tests are a challenge, for both author and reviewer.)

Done.

---

tools/install/install.py.in, line 143 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Wrong level of intendation -- should be only +4 versus the if this body is filling in.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

+(status: curate commits before merging) The commits bled into each other a bit. FYI Just doing a squash in GitHub merge process here is OK by me.

modulo the one open discussion below (filing an issue).

---

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jwnimmer-tri]

-(status: curate commits before merging)

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[fbudin69500]

+@sammy-tri Platform review please

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[sammy-tri]

Reviewed 2 of 4 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

tools/install/install.py.in, line 162 at r6 (raw file):

            os.path.relpath(libs[soname][0], lib_dirname)
        )
        rpath.add('$ORIGIN' + '/' + diff_path)

I'm confused here... Are we deleting rpath entries with $ORIGIN in them in the mac version, and then adding rpaths with $ORIGIN in the linux version? I think a couple of comments describing a typical "before" and "after" rpath in each case might be helpful. (I've got a decent grasp of ELF dynamic linking but there's a lot of context at this point of execution of the script which I'm not familiar with, sorry... )

---

Comments from Reviewable

[fbudin69500]

Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

tools/install/install.py.in, line 162 at r6 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I'm confused here... Are we deleting rpath entries with $ORIGIN in them in the mac version, and then adding rpaths with $ORIGIN in the linux version? I think a couple of comments describing a typical "before" and "after" rpath in each case might be helpful. (I've got a decent grasp of ELF dynamic linking but there's a lot of context at this point of execution of the script which I'm not familiar with, sorry... )

On MacOS, we do not use RPATH as the path to each library is individually set (relative or absolute depending if it is part of the package or a system library). On linux, libraries are found using RPATH. The RPATH values used in the build tree are removed with the check_output call below while the new RPATH values corresponding to the install tree are written. Let me know if the new comments are clearer.

---

tools/install/install.py.in, line 156 at r7 (raw file):

        if len(ldd_result) < 2 or \
                not (ldd_result[1] == "not found"
                     or ldd_result[1].startswith(prefix)):

Added a test here to make sure that any library that is has a path containing the prefix also has its relative path to the current target also updated, just in case.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[sammy-tri]

---

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

tools/install/install.py.in, line 162 at r6 (raw file):

Previously, fbudin69500 (Francois Budin) wrote…

On MacOS, we do not use RPATH as the path to each library is individually set (relative or absolute depending if it is part of the package or a system library). On linux, libraries are found using RPATH. The RPATH values used in the build tree are removed with the check_output call below while the new RPATH values corresponding to the install tree are written. Let me know if the new comments are clearer.

I think that's a bit clearer, thanks

---

Comments from Reviewable

[fbudin69500]

I filed an issue for the lack of testing for install.py.in : #7331

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jamiesnape]

---

Reviewed 2 of 4 files at r3, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable