mujoco
mujoco copied to clipboard
google-deepmind
Remove uselsess TINYOBJLOADER_IMPLEMENTATION definition
As far as I understand, TINYOBJLOADER_IMPLEMENTATION macro is used to be defined in libraries or executables that consume tinyobjloader only including its header, without linking the corresponding library. However mujoco (at least with the CMake build system provided in this repo) is already linking the tinyobjloader library in which all the symbols are defined, so there is no need to re-defined them.
I am not fully 100% sure about the reasoning behind the PR, so one of the reasons for opening the PR is indeed to check if the CI works correctly even by removing the TINYOBJLOADER_IMPLEMENTATION definition.
OK so I've now run this on our internal CI, and this is in fact required for our internal build process that we use to generate our official binaries.
The prebuilt library that we ship contains all of MuJoCo's dependencies statically linked in, but with symbols hidden. We still believe that this is the cleanest and safest way for us to ship prebuilt libraries (we also target GLIBC_2.17 aka manylinux2014 aka CentOS 7, and our libmujoco.so doesn't have any exposed C++ ABI surface -- it has its own copy of libc++ baked in, and also hidden). This means that in our build we want that file to provide tinyobjloader symbols.
The correct "fix" for this would be to define a CMake variable (MUJOCO_STATIC_TINYOBJLOADER or something like that) that translates to a macro that controls whether TINYOBJLOADER_IMPLEMENTATION is defined.
The correct "fix" for this would be to define a CMake variable (
MUJOCO_STATIC_TINYOBJLOADERor something like that) that translates to a macro that controls whetherTINYOBJLOADER_IMPLEMENTATIONis defined.
Sorry, I had missed that. I will look into this, thanks!
@saran-t I tried to implement in the simplest way possible your suggestions.
What happens with this PR is the following:
- If MuJoCo is built with CMake,
TINYOBJLOADER_IMPLEMENTATIONis not defined - When MuJoCo is built with some other build system not mantained in this repo such as your internal build process,
TINYOBJLOADER_IMPLEMENTATION
Is this ok for you? I kind of made the assumption that your internal build process is not based at all on CMake (I guess is blaze/bazel-based), but please tell me if this assumption is wrong.
@saran-t I tried to implement in the simplest way possible your suggestions.
What happens with this PR is the following:
* If MuJoCo is built with CMake, `TINYOBJLOADER_IMPLEMENTATION` is not defined * When MuJoCo is built with some other build system not mantained in this repo such as your internal build process, `TINYOBJLOADER_IMPLEMENTATION`Is this ok for you? I kind of made the assumption that your internal build process is not based at all on CMake (I guess is blaze/bazel-based), but please tell me if this assumption is wrong.
Hello @saran-t, I was doing a cleanup of PRs that I had open, are you still interested in this change? If yes I can keep the PR open, otherwise perhaps I would close it to cleanup my PRs list. Thanks a lot in advance!