search

opentomb / OpenTomb

An open-source Tomb Raider 1-5 engine remake
http://opentomb.github.io/
GNU Lesser General Public License v3.0
1.39k stars 146 forks source link

Handling of external libraries #115

Open cochrane opened 9 years ago

cochrane commented 9 years ago

We need to converge with this discussion somewhere because right now, it is all over the place. So here are the possibilities for handling the dependencies our code has that I've been able to think of:

A few more notes:

vvs- commented 9 years ago

This is exactly what we need.

No it isn't. It controls building of shared libraries but not static.

vvs- commented 9 years ago

Please check recent changes - I have removed Bullet and freetype

Yes, now CMakeLists is broken. The commands to find them were in 3rdparty/CMakeLists.

Lwmte commented 9 years ago

Sorry for this, I thought this CMakeLists file was only used if bullet/freetype were added as submodules. Also I wasn't aware of current engine state, as it's first time I've compiled it since the big merge. I just saw your screenshot and thought that texture and animation issues were already fixed. Regarding CMake static/dynamic library linking - then how can we explicitly tell it that all live must be static?

Lwmte commented 9 years ago

Sorry, libs, not live. Stupid autocorrection.

vvs- commented 9 years ago

I thought this CMakeLists file was only used if bullet/freetype were added as submodules.

And I think that's exactly why it was a wrong decision to put it there.

I just saw your screenshot and thought that texture and animation issues were already fixed.

We need them fixed and Steffen has no knowledge of how the old version actually worked. So, we need help from all the developers with that knowledge.

Regarding CMake static/dynamic library linking - then how can we explicitly tell it that all live must be static?

I don't know yet. We'll need to consult the manual :smile:

Lwmte commented 9 years ago

Ok then, I'll see what I can do when I get home :smiley:

vvs- commented 9 years ago

add_library command allows to specify which type of library should be built.

"_If no type is given explicitly the type is STATIC or SHARED based on whether the current value of the variable BUILD_SHAREDLIBS is ON."

So, it builds static libraries by default. And the command find_library can be used to add existing libraries to the linker list.

vvs- commented 9 years ago

Here's an example: 

add_library(l SHARED l.c)
add_executable(e e.c)
target_link_libraries(e l)

This project includes one executable and one library. It compiles both, builds shared library and links executable dynamically. To build a static library and link statically, just remove SHARED.

vvs- commented 9 years ago

And I see that there is a convenient function trylinklib in OpenTomb already. In the current CMakeLists it tries to find shared versions of several libraries but that could be changed. But such change should be encapsulated in a platform specific manner because not every platform has static versions of all libraries.

stohrendorf commented 9 years ago

Proposal (and the only sane way): Only list libs that we require directly, i.e. at first hand. Transient dependencies are handled by the respective package maintainers, and if one wants to compile these libs by hand... well, these libs have listed their deps for themselves.

vvs- commented 9 years ago

That's a good idea for unices. But I doubt it would please developers on Windows.

stohrendorf commented 9 years ago

Developers on Windows are always unpleased, unless they use VS, but then they are only pleased if everybody develops only on windows using VS. By the way, OpenTomb already uses C++11 features that VS still doesn't support. And MSYS2 has a really neat package manager (pacman) that does its job really good, and let me set up a dev env on windows unbelievable easy compared to trying to setting it up using VS.

vvs- commented 9 years ago

That means we are back to discussion about good development environment for OpenTomb. Then Windows developers should be pleased once and for all :wink:

Lwmte commented 9 years ago

Oh, there is MSYS2 already? :open_mouth:

Talking about dependencies... Are there wrong entries currently in CMakeLists.txt? I'm worrying primarily about sound libs. sndfile requires ogg/vorbis libs explicitly, or they count as dependencies? If so, we can safely remove everything except linking sndfile itself.

vvs- commented 9 years ago

As far as I understand, cmake counts only explicit library dependencies listed in CMakeLists. At least I was not able to link my test example above without it.

stohrendorf commented 9 years ago

sndfile requires ogg/vorbis, but it needs the target program to link against them. That's also stated on the homepage.

cochrane commented 9 years ago

Sorry I missed out on the discussion over the last couple of days.

Proposal (and the only sane way): Only list libs that we require directly, i.e. at first hand. Transient dependencies are handled by the respective package maintainers, and if one wants to compile these libs by hand... well, these libs have listed their deps for themselves.

Who are the package maintainers in your scenario? As far as I understand it, I'm the one for the Mac version, almost everybody else is managing the Windows version. There is no external package maintainer role that we can shift the responsibility to.

This proposal, and all the related things, mean that I either have to set up a zip file with all the libraries required for Mac somewhere and tell every Mac developer to download it in addition to the code. Alternatively, I can just say "You have to jump through at least hoops if you want to help us", meaning telling them to install random versions of the library themselves. That may indeed be the only practical way on Linux, where everyone has a randomly different operating system, but I have never heard of it as best practice anywhere else.

The solution of including submodules sometimes and not other times does work, but it seems very needlessly complex.

stohrendorf commented 9 years ago

Does that mean that Mac OS X handles packages like windows, i.e. every app has its own copy of the libs? I thought there was a dependency system like in DEB or RPM, too.

cochrane commented 9 years ago

No, there is no dependency system like that. It's like Windows in that regard.

Unlike Windows, it's easy to install your own dependency manager, and there are several around, but it's still only something for power users. We can't expect normal people to install OpenTomb that way. Or at least I don't want that, I want it to be a normal OS X app, because that's a much better user experience.

stohrendorf commented 9 years ago

So what do you propose then? I can only speak for windows and linux.

cochrane commented 9 years ago

What I said at the start: Include the source code as external references, or the pre-built libraries (far less preferred).

I realize this isn't perfect for Linux, so I'm open for better suggestions. If OS X had a built-in package manager, I would probably agree with you that using that would be the best option.

I think the external references would work well on Windows, too.

stohrendorf commented 9 years ago

What about putting a DEPENDENCIES file into the source tree, with links to archives with the correct version? I'd personally prefer that instead of using submodules, because then you are less tempted to patch around in the libs.

cochrane commented 9 years ago

Hm… I don't like it at all, because it's annoying, but it would certainly work. With the list in the Wiki, we do have most of the work for that done already. Still, it's annoying.

I think I'm going to do some research on how other open-source projects with a Mac version (as opposed to Mac-only open source projects) handle this issue, and report back.

stohrendorf commented 9 years ago

Are there any points unresolved? I'd like to close this, as the system as it is right now seems to work.

cochrane commented 9 years ago

I still haven't had time to do the researcher ruined above, so I'd prefer to keep this open

cochrane commented 9 years ago

Damn autocorrect. You know what I mean.

stohrendorf commented 9 years ago

I have seen a bundle script for app bundles in cmake, which (according to what I understood) should do all the bundling automatically: http://www.cmake.org/cmake/help/v2.8.12/cmake.html#module:BundleUtilities

Lwmte commented 9 years ago

I still don't get an idea of referencing submodule of Bullet in CMake, while we don't have one in actual source code.

vvs- commented 9 years ago

You might be missed this then.

stohrendorf commented 9 years ago

Probably the best way to handle bullet is this:

vvs- commented 9 years ago

This is too heavy considering that nobody will ever use it except OpenTomb. And it builds too much stuff that OpenTomb doesn't use.

cochrane commented 9 years ago

No offense, but the above is exactly the reason why I think the current situation is not an acceptable resolution to this issue.

So I did the research, and among serious projects I found two main ways: One is to just out the requirements in a readme, and let developers deal with it, used e.g. By Wings3D. The other is to include the source code either directly (Blender) or in a separate repository (Ogre3D). Some put everything into their SVN repository, which I think is just ugly.

The current "just put it in a readme" approach is okay on systems with a package manager, where installing the prerequisites is fast and easy. In reality, the systems used by most people and most developers here are ones that don't have that. Installing these requirements by hand is annoying as hell, especially when people keep adding new ones.

So I think we absolutely need the libraries in one place in our project. Can be as submodules so you don't patch around in them (have you ever tried patching around in a submodule? You'll realize the error of your ways very quickly).

stohrendorf commented 9 years ago

@vvs- Then the cmake command line for bullet needs to be extended with -DBUILD_BULLET3=OFF -DBUILD_CPU_DEMOS=OFF -DBUILD_EXTRAS=OFF -DBUILD_OPENGL3_DEMOS=OFF -DBUILD_SHARED_LIBS=OFF -DBUILD_UNIT_TESTS=OFF. That's what works in windows, and it's a minimal build. I'm just concerned that the other approach of removing the system provided libs leads to a system that's too specialized.

@cochrane I could live with bullet as a submodule, as long as it's a bit abstracted. I think of something like a "pre-configure step" to separately build bullet in-tree, with the commands state above. I.e., when doing the first cmake on OpenTomb, bullet would be automatically built, and then used. I'll try that and maybe put on a merge request later.

cochrane commented 9 years ago

Why only bullet?

stohrendorf commented 9 years ago

The pull request is open (#247), and is a "works for me version" (TM). Should be easily extensible to other libs that use cmake.

vvs- commented 9 years ago

removing the system provided libs leads to a system that's too specialized.

And if you don't then you are risking to inadvertently bring different versions of headers or even libraries.

stohrendorf commented 9 years ago

If the compilers don't respect local includes over system includes, then the compilers are not standard-compliant (and crap). The approach I tried in #247 should prefer local includes and libs from the locally built bullet over the ones provided by the system.

vvs- commented 9 years ago

That works only if the project file doesn't have bugs. And if it does that leads to a very obscure brokenness which is hard to track down. I dealt with such cases numerous times and can assure that they are quite common.

stohrendorf commented 9 years ago

So you're saying that #247 probably doesn't solve any problems, but instead makes them more obscure?

vvs- commented 9 years ago

I never said anything about #247. I even didn't look at it yet. What I meant is that it's very unreliable to count on the paths and that they will be always in correct order. Better take no chances and never install several versions of the same library unless you have some sort of a chroot or a container.

stohrendorf commented 9 years ago

Sorry, I misunderstood you, and I agree with you. But from looking at the code, we don't use the bullet/ path prefix for the includes anymore, which means that the include root path must be specified on the command line, so at least here is no way the compiler may confuse the pathes. And the bullet libs are qualified using their absolute file pathes, so here should be no problem, too. (I'm primarily promoting #247 here, but it's valid for the current master branch as well.)

OpenAL is handled the same way; e.g., my CMakeCache.txt contains:

OPENAL_INCLUDE_DIR:PATH=/usr/include/AL
OPENAL_LIBRARY:FILEPATH=/usr/lib64/libopenal.so

BULLET_COLLISION_LIBRARY:FILEPATH=/home/sto/devel/OpenTomb-bullet/3rdparty/bullet3/install/lib/libBulletCollision.a
BULLET_COLLISION_LIBRARY_DEBUG:FILEPATH=BULLET_COLLISION_LIBRARY_DEBUG-NOTFOUND
BULLET_DYNAMICS_LIBRARY:FILEPATH=/home/sto/devel/OpenTomb-bullet/3rdparty/bullet3/install/lib/libBulletDynamics.a
BULLET_DYNAMICS_LIBRARY_DEBUG:FILEPATH=BULLET_DYNAMICS_LIBRARY_DEBUG-NOTFOUND
BULLET_INCLUDE_DIR:PATH=/home/sto/devel/OpenTomb-bullet/3rdparty/bullet3/install/include/bullet
BULLET_MATH_LIBRARY:FILEPATH=/home/sto/devel/OpenTomb-bullet/3rdparty/bullet3/install/lib/libLinearMath.a
BULLET_MATH_LIBRARY_DEBUG:FILEPATH=BULLET_MATH_LIBRARY_DEBUG-NOTFOUND
BULLET_SOFTBODY_LIBRARY:FILEPATH=/home/sto/devel/OpenTomb-bullet/3rdparty/bullet3/install/lib/libBulletSoftBody.a
BULLET_SOFTBODY_LIBRARY_DEBUG:FILEPATH=BULLET_SOFTBODY_LIBRARY_DEBUG-NOTFOUND

(By the way, I have the debug versions of Bullet installed in the system, but CMake hasn't used them, since BULLET_ROOT was used.)

vvs- commented 9 years ago

Of course, I don't say that it will necessary happen, but the risk is still there. In fact, the old CMake project included different Bullet paths twice and nobody even noticed that. So, it's 50/50 and I don't like that risk. I still suggest to always remove any alternate versions before compiling in-tree.

stohrendorf commented 9 years ago

I think this discussion became somewhat subjective...

I agree that multiple installations can be a problem, and I think your aproach of removing the system libs is a good suggestion if something seems wrong, but I don't believe that the chance of such a misconfigured build environment is at 50%. (Hell, I believe that #247 works in over 90% of the cases, and I hope I'm not wrong o.O)

Sur3 commented 6 years ago

I don't see a reason to include code from libraries in an own project, cmake should handle finding the system libraries if installed and if not warn about it missing. If modifications to external libraries are really needed you could clone the external source in an own git and set a tag to that and also commit a pull request upstream. And also an option to use the system library instead in cmake should be included in that case, probably hinting what features are disabled if that is used instead of the bundled version. But changing libraries without upstream pull requests or possibility to use system libraries instead is always a bad idea; I started developing supertuxkart recently and they have their own version of the Irrlicht engine which was pretty confusing because the changes are not documented well and if searching for Irrlicht tutorial they don't fully apply to that code.. The same problem goes for every project duplicating libraries making local changes.

vvs- commented 6 years ago

I agree with your conclusion. But an amateur projects just don't have a required discipline or incentive to follow recommended best practices. That's just a fact with which we have to live on.