Remove absolute path from libpython.dylib

[keith]

Previously this set the install name of the binary, which is put into
any binaries that link this, as an absolute path that is user specific.
Instead this can be fetched relatively, and bazel will automatically
handle adding the correct rpaths for this.

[keith]

cc @nicholasjng

[nicholasjng]

How exactly does Bazel set the rpath? Does it add every location with cc_imports in it to the rpath list?

In any case, this is better than my previous solution, which ultimately just traded a non-existing absolute lib path against an existing one.

[keith]

How exactly does Bazel set the rpath? Does it add every location with cc_imports in it to the rpath list?

Yea something like this, in general it's an implementation detail but as long as it's correctly surfaced through the CC rules it's up to bazel to handle.

In any case, this is better than my previous solution, which ultimately just traded a non-existing absolute lib path against an existing one.

Yea I guess since the path was invalid before, it's unlikely that folks were able to depend on it. But importantly the first non-existant path was stable, and this absolute one means teams won't get cache hits if they are using it.

[rickeylev]

Yeah, its still a net improvement, so LGTM.

bazel will automatically handle adding the correct rpaths for this.

Is it bazel doing this (i.e. special behavior in the CC rules), or simply the linker handling @rpath variables? Mostly just curious.

Doing this in the repo-phase isn't really ideal, but I don't know enough about the C/C++ toolchains, or the Mac specific parts of them, to know how to do it during the build phase. Ideally, this would be better done during the build phase. e.g. something like: look up the install_name_tool via toolchains, run it on the original dylib, then output the modified one, then that modified one is what is put into the cc_whatever() target. The basic win from doing it this way is cross-build support.

[keith]

Is it bazel doing this (i.e. special behavior in the CC rules), or simply the linker handling @rpath variables? Mostly just curious.

bazel is doing it.

Doing this in the repo-phase isn't really ideal, but I don't know enough about the C/C++ toolchains, or the Mac specific parts of them, to know how to do it during the build phase.

we have a rule internally I've been using to do this during the build. But the problem with that today is that you can end up with 2 libpython's in the same process, which then leads to runtime issues with global variables in python.

This is technically a bit sketchy today since install_name_tool is dependent on the host's Xcode version, but realistically the differences there likely won't matter. This also invalidates code signatures, but also likely doesn't matter in practice.

The most ideal solution IMO would be to fix this in how this libpython is built, the linux version is doing this out of the box, so it seems like the macOS version could too

[rickeylev]

How would you end up with 2 libpythons being loaded...? There's still just one libpython?

The build-phase solution I had in mind was something akin to:

# repo-phase:
mv("libpython.dylib", "libpython.dylib.orig")
# BUILD
genrule(
  name = "fixlibpython",
  srcs = "libpython.dylib.orig",
  outs = "libpython.dylib",
  cmd = "$install_tool -id rpath/libpython $(SRCS) $(OUTS)",
)

cc_library(name='libpython', srcs="libpython.dylib")

Basically just doing what the repo is doing at build time instead. No idea how to get a reference to install_tool (or some equivalent), though. It's probably part of some toolchain or apple thing somewhere, though.

[keith]

How would you end up with 2 libpythons being loaded...? There's still just one libpython?

in our case the rule to fix the install_name was in our project, and then we would point things to that, but python3 itself would still load the old one i think? vs this change overriding the original one