Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit d627374 introduces binary incompatibility that breaks building the python bindings from source #217

Closed
aftersomemath opened this issue Mar 31, 2022 · 12 comments

Comments

@aftersomemath
Copy link
Contributor

aftersomemath commented Mar 31, 2022

d627374 changes an enum, however the shared library files released with in version 2.1.3 presumably use the earlier version of that enum.

Building the python bindings using the headers released with 2.1.3 results in a compilation error due to the changed enum. If the headers from main are used instead, compilation succeeds, but python -c "import mujoco" segfaults, presumably due to binary incompatibility with the released libmujoco.so.2.1.3.

I tested that the commit before d627374 (no error), d627374 (error), and the tip of main (error).

@aftersomemath aftersomemath changed the title Commit d627374 introduces binary incompatibility that breaks building the python bindings from source Commit d6273745459ef1402a52a5c79961ff3648ccb596 introduces binary incompatibility that breaks building the python bindings from source Mar 31, 2022
@aftersomemath aftersomemath changed the title Commit d6273745459ef1402a52a5c79961ff3648ccb596 introduces binary incompatibility that breaks building the python bindings from source Commit d627374 introduces binary incompatibility that breaks building the python bindings from source Mar 31, 2022
@saran-t
Copy link
Member

saran-t commented Mar 31, 2022

My bad, I was cherry picking commits and thought this one was a simulate-only change. I'll revert it shortly. Sorry about that.

@saran-t
Copy link
Member

saran-t commented Mar 31, 2022

We've created something of a mess for ourselves around simulate 😅. Before the revert the GitHub version is the same as our internal version, but that's not true anymore. As things stand it's going to be a real pain to get this PR reviewed against our internal HEAD for obvious reasons, so we're planning to do a tiny 2.1.4 binary release, hopefully on Monday, just so that we can put the repos back in sync again.

Let's hold off further review until that's done? Sorry about that.

@aftersomemath
Copy link
Contributor Author

Lol, I thought something like this would happen. No problem, I'll delay PRing bindings for simulate till then and finish them by working off the tip of main without rebasing #201. Thanks for the quick response.

@saran-t
Copy link
Member

saran-t commented Apr 4, 2022

v2.1.4 is out now

@aftersomemath
Copy link
Contributor Author

v2.1.4 breaks simulate. It compiles but on launch prints ERROR: gladLoadGL error and opens a blank window.

Based on GLAD's README I think gladLoadGLLoader needs to be called in simulate. It seems like this could be done if the generated glad.h used to build libmujoco was included in the release archive. Is that an option?

@aftersomemath
Copy link
Contributor Author

If you want I can make a separate issue for this.

@saran-t
Copy link
Member

saran-t commented Apr 4, 2022

Can you confirm the setup that you're having issue with? I've successfully launched simulate on all of our supported platforms before I pushed out the release so am somewhat surprised to hear this.

@saran-t
Copy link
Member

saran-t commented Apr 4, 2022

OK, as far as I can tell the issue appears to be with the Makefile for Linux (we don't use that Makefile for our prebuilt binary so didn't catch the issue -- the simulate binary that we ship does seem to work correctly). For some reason the -lGL isn't introducing DT_NEEDED libGL.so on simulate. Still investigating, but that ought to give you a bit more clue too.

@saran-t
Copy link
Member

saran-t commented Apr 4, 2022

Alright, looks like we need to tweak the GLAD setup on our side. We've fairly heavily modified the GLAD loading logic to allow for auto-switching between OSMesa, EGL, and GLX, and also to support our Google-internal setup.

The root cause is that because neither simulate nor libmujoco.so depend explicitly on OpenGL symbols anymore, the linker can decide that -lGL is actually not needed and just drop it the dynamic dependency list. This seems to occur with GNU ld, but not with LLVM's lld. I'll try to make changes in MuJoCo for the next release to make things more robust, but for now you have two solutions:

  • Pass -Wl,-no-as-needed to g++ when linking; or
  • Switch to clang++

@aftersomemath
Copy link
Contributor Author

Great thanks for the quick response.

-Wl,-no-as-needed fixed simulate for gcc.

I would be happy to switch to clang, is mujoco going towards being a clang only project?

I am having a similar problem with GNU's ld dropping the glew symbols when compiling the _simulate python module. This probably will help with that too, thanks!

@saran-t
Copy link
Member

saran-t commented Apr 4, 2022

We're likely only going to run our CI/CD against LLVM/Clang, but will try to make sure that MuJoCo remains portable on a best-effort basis (although this will likely rely on the community helping us fix breakages that do not affect LLVM-based builds).

@aftersomemath
Copy link
Contributor Author

Great, that will go a long way to keeping the barrier to entry low. Would a travis build using gcc be welcome in the communities version?

kevinzakka pushed a commit to kevinzakka/mujoco that referenced this issue May 24, 2022
This commit requires a new binary build, and therefore breaks compatibility with version 2.1.3.

Fixes google-deepmind#217.

Change-Id: I1e08b6fd2c468e3845e2166a1e4cc9d265de64de
kevinzakka pushed a commit to kevinzakka/mujoco that referenced this issue May 24, 2022
This commit requires a new binary build, and therefore breaks compatibility with version 2.1.3.

Fixes google-deepmind#217.

Change-Id: I1e08b6fd2c468e3845e2166a1e4cc9d265de64de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants