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

Allows building using Emscripten and 'WITH_BULLET' set #53

Closed
wants to merge 8 commits into from

Conversation

@bowling-allie
Copy link
Contributor

bowling-allie commented Sep 12, 2019

if 'EMSCRIPTEN' is on, 'find_package(Bullet)' and others are replaced
with 'target_compile_options(... -s USE_BULLET=1)' effectifely using
the emscripten-ports/Bullet library

if 'EMSCRIPTEN' is on, 'find_package(Bullet)' and others are replaced
with 'target_compile_options(... -s USE_BULLET=1)' effectifely using
the emscripten-ports/Bullet library
@bowling-allie

This comment has been minimized.

Copy link
Contributor Author

bowling-allie commented Sep 12, 2019

the build might have failed because there were 4 jobs running?
one job downloaded the port(bullet), another other one didnt and errored when it saw an include
kinda doesnt make sense tho
builds on my machine using wsl Ubuntu 18.04.3 LTS

@mosra mosra added this to the 2019.0b milestone Sep 12, 2019
@mosra mosra added this to TODO in Physics via automation Sep 12, 2019
@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 12, 2019

Hi, thanks a lot for your contribution!

(I must admit I think Emscripten is doing maybe a bit too much by having random library ports available through a compiler switch, hah 🙂)

the build might have failed because there were 4 jobs running?

Yeah, it seems so. There are two *.cpp files, so while it was downloading the port for the first of them, the second happily started building in parallel, dying because the files were not there yet. I encountered similar problems myself. Do you know if there's a direct way to tell Emscripten to download the port (without having to compile a file with -s USE_BULLET=1)? We could do that on the CI before the actual build.

Looking at https://github.com/emscripten-ports/Bullet, it seems to be last updated in 2015 (Bullet 2.82), while this is a nice convenience thing to have (instead of forcing everyone to build Bullet themselves), I think it should allow the users to use a custom-built non-outdated version as well. Will think about the best way how to expose this under some option.

@bowling-allie

This comment has been minimized.

Copy link
Contributor Author

bowling-allie commented Sep 12, 2019

I believe you can use embuilder.py to build and cache libraries.
From the help text:

You can use this tool to manually build parts of the emscripten system
environment. In general emcc will build them automatically on demand, so you do not strictly need to use this tool, but it gives you more control over the process (in particular, if emcc does this automatically, and you are running multiple build commands in parallel, confusion can occur

so embuilder.py build bullet would do that

Maybe just a cache variable to use the port? like USE_EMSCRIPTEN_PORTS_BULLET? Its a little lengthy, but if its default is OFF the build is same as it ever was. Just has the convenience for someone who doesn't want to put a lotta effort into building.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 12, 2019

Nice, thanks for digging that up. 👍 I would maybe even make that variable ON by default, since 90% people would be fine with Bullet from 2015 anyway I think :)

I'm a bit overloaded currently and don't have time to give this a try right now (sorry)... If you want and have time, you can try calling embuilder.py on the CI (the driver file is in package/ci/travis-emscripten.sh), with a bit of luck embuilder.py could be already in PATH so it would be just about adding that one line there. If you don't, no worries, I'll have a look at this early next week.

on by default
if o, passes '-s USE_BULLET=1' to emcc instead of asking cmake for
the lib (possibly an old bullet build)
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 13, 2019

Codecov Report

Merging #53 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #53   +/-   ##
=======================================
  Coverage   74.59%   74.59%           
=======================================
  Files          21       21           
  Lines         874      874           
=======================================
  Hits          652      652           
  Misses        222      222

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb83cba...f7692ca. Read the comment docs.

@bowling-allie

This comment has been minimized.

Copy link
Contributor Author

bowling-allie commented Sep 13, 2019

oof.
Ok I never tried it with test on. oops.
Seems like bullet3 copies a header or two and also seems like the emscripten-ports bullet just uses the src tree as the include dir... weird

Also also, emscripten-ports/bullet lib is missing symbols(?) or features, or is too old for the tests anyway.

So, I'm going to undo the ci changes, set the default behavior to off, because it still works without the tests, and a warning about that if its turned on

also added a warning if it is turned on that it may not be compatable
with tests
@bowling-allie

This comment has been minimized.

Copy link
Contributor Author

bowling-allie commented Sep 13, 2019

Ya, the emscripten-ports/bullet lib is from 2013. It's just too dang old for the tests

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 18, 2019

Hi, sorry for the delays on my end --

just to give you an update: I'm in process of merging this, just wanted to expose the feature to FindMagnumIntegration.cmake as well so the USE_BULLET flag gets propagated also with separate installations and not only CMake subprojects. I have to run now but will have that done by tomorrow. And I'll have a look at mosra/toolchains#8 as well :)

@bowling-allie

This comment has been minimized.

Copy link
Contributor Author

bowling-allie commented Sep 18, 2019

Thank you!

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 20, 2019

Sorry for the constant delays :) ... extended version merged as 4328706. Turns out the missing header in tests was present in two places and the other place is available consistently everywhere. Not sure why is it duplicated like that, but most importantly the tests pass also with this ages old version. (But I didn't enable it on the CI, I still prefer the new version more.)

The -s USE_BULLET=1 flag unfortunately needs two features present in CMake 3.12 and 3.13 (e.g., when combined with -s USE_WEBGL=2, CMake would happily turn it into -s USE_BULLET=1 USE_WEBGL=2 and, ... well. This is one point of totally unnecessary extreme suffering I hate about Emscripten (it could accept -sUSE_BULLET=1 and everything would be fine again but it doesn't), hopefully this increased version requirement for this particular feature is not a problem for you.

Also added this to the docs (and you to credits). Thanks for the contribution! :)

@mosra mosra closed this Sep 20, 2019
Physics automation moved this from TODO to Done Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Physics
  
Done
3 participants
You can’t perform that action at this time.