Added an option to allow building OIS as static library.

#5 Declined
Repository
klaim
Branch
default
Repository
cabalistic
Branch
default
Author
  1. A. Joël Lamotte (Klaim)
Reviewers
Description

I'm using OIS in my project using Ogre, I just want OIS to be built in static mode, whatever the platform. So I made an option.

I didn't check see if there is other libs that would benefit from such an option.

Comments (14)

  1. X

    CMake already provides a facility for this: BUILD_SHARED_LIBS. There's no reason for each library to invent its own switch and logic here.

    From the documentation for add_library:

    If no type is given explicitly the type is STATIC or SHARED based on whether the current value of the variable BUILD_SHARED_LIBS is true.

    So I'd like to see us remove the SHARED parameter to add_library instead. That gives users a uniform way to control this kind of thing for any library.

  2. Holger Frydrych repo owner

    Interesting, didn't know about that. In practice, though, it's never quite that easy. As you can see in the diff below, just before the add_library we have to define OIS_DYNAMIC_LIB for shared libs. This is probably irrelevant for Unix platforms, but will cause problems on Windows if the flag is set (or not set) incorrectly. So we may not need an extra config option, but we do still need an if.

  3. A. Joël Lamotte (Klaim) author

    mirzagaribovic> The problem is that the OIS project generation happen in Ogre CMake files, not in mine, so I can't turn BUILD_SHARED_LIBS on and off between OIS calls, I need to have the control as an option from Ogre point of view. AFAIK there is no CMake standard way to specify for a specific library it's linkage at another moment than on the add_library() call. BUILD_SHARED_LIBS don't seem enough control unfortunately.

  4. X

    Holger, I see. You're right, we'll still need an if to get that definition right. This patch should address that before being pulled (because it's still adding that definition regardless of the library type). If OIS_FORCE_STATIC_BUILD is on, it should just take the existing path that uses STATIC instead of doing all this work.

    Joël, I know your pain, Ogre's CMake setup is notoriously complicated. It would be great if we could prevent that complexity from leaking into other libraries. If you feel that linking OIS itself statically is important, we could just as easily add this switch to Ogre instead, such that it configures BUILD_SHARED_LIBS appropriately before handing it down to OIS.

  5. A. Joël Lamotte (Klaim) author

    I'm not sure of how you want to do this? I mean: I am not making Ogre using OIS. To make it short: I call add_subdirectory() on my ogre dependencies sources directory, then I use it's address as Ogre's CMake option to make it use the dependencies when I call add_subdirectory() on ogre source dirs. But I disable samples, because it's my app that needs OIS, not Ogre. So, the goal is just to allow me, ogre dependencies source user, to set OIS to static if I want, but only it. It's not clear to me how modifying ogre will help? I certainly don't understand what you suggest. Also, aren't these CMake files specific to ogre dependencies repo?

  6. X

    When you said that "OIS project generation happen in Ogre CMake files", I assumed you were referring to how Ogre automatically finds and includes ogredeps if it exists under ./ogredeps or ./Dependencies. That path would make it hard to set BUILD_SHARED_LIBS for OIS only. If you're using OIS directly, you can configure BUILD_SHARED_LIBS without changing OIS. Like this:

    set(BUILD_SHARED_LIBS NO)
    add_subdirectory(ois)
    

    This doesn't require any special switches for OIS.

    It's not clear to me how modifying ogre will help? I certainly don't understand what you suggest. Also, aren't these CMake files specific to ogre dependencies repo?

    Here's what I was suggesting:

    • OIS itself should be agnostic of Ogre/ogredeps.

    • Any complexity in Ogre/ogredeps that prevents you from configuring OIS should be localized to Ogre/ogredeps and not leak through to OIS.

    It follows then that instead of adding glue code like this to OIS, we should move it up a level to the code that's adding the complexity which prevents you from configuring OIS.

    For example:

    A. ogre/CMakeLists.txt
        B. includes ogre/ogredeps/CMakeLists.txt
            C. includes ogre/ogredeps/src/CMakeLists.txt
                D. includes ogre/ogredeps/src/ois/CMakeLists.txt
    

    In this case, C. would be a fine place to declare OIS_LIBRARY_MODE/OIS_FORCE_STATIC_BUILD and then set BUILD_SHARED_LIBS. That would allow you to choose to make only OIS static and keep everything else dynamic/whatever it happens to be.

  7. A. Joël Lamotte (Klaim) author

    Ah yes, ok I see now, better design indeed! As far as the option is still available to me and works, I'm ok with any implementation and it's still far better than to have to add it in OIS cmake files.

    Will you do it or do you want me to fix the PR myself?

  8. X

    I'm brand new to hg/bitbucket so I'm a little clueless still. If Holger decides that having OIS or per project static/dynamic switches makes sense, it should be pretty easy to move up.

    There's also the question of platform-specific logic (e.g. iOS always requires static and dynamic doesn't work). Since every library has to deal with that, I think none should and we should just set that at the top level somewhere (e.g. A or B up there), or let the toolchain configuration or end user set a correct default for BUILD_SHARED_LIBS.

  9. A. Joël Lamotte (Klaim) author

    I agree. However, note that only OIS have this SHARED by default on non-IOS platforms. All other libraries are never in SHARED. Which made me think at the time that an option only for OIS would be helpful. The other libraries are part of the OgreMain binary anyway. OIS not being a real requirement...

  10. X

    However, note that only OIS have this SHARED by default on non-IOS platforms. All other libraries are never in SHARED.

    Aaaah. Weird. Makes sense that you would want to change that.

  11. A. Joël Lamotte (Klaim) author

    Here is the other implementation of the same functionality: https://bitbucket.org/klaim/ogredeps-ois-option-2 (forked from this PR) I find more verbose and obscure than the one in this PR though... One of the problem is that changing a global state variable to change the behaviour of a function is a great source of future problems. I think this PR is a better solution. After all, it's OIS CMake file that forces DLL by default by having SHARED. Also this CMake file, AFAIK, is specific to this Ogre Dependencies repository, not the OIS repo, right?