search

wjakob / nanogui

Minimalistic GUI library for OpenGL
Other
4.66k stars 608 forks source link

Conan package for nanogui #301

Open zamazan4ik opened 6 years ago

zamazan4ik commented 6 years ago

Hello, Do you know about Conan? Conan is modern dependency manager for C++. And will be great if your library will be available via package manager for other developers.

Here you can find example, how you can create package for the library.

If you have any questions, just ask :-)

melMass commented 6 years ago

I tried to conan package it myself but kept having issues with nanovg linking. Would love to see a working package as it perfectly fit the libary to be on a package manager.

zamazan4ik commented 6 years ago

What kind of problems did you get? do you have a link for your recipe?

melMass commented 6 years ago

Yes I actually managed to make it work on OSX. I'm pretty new to conan so be gentle. But it works here. I have to figure out how to assign the dependencies to nanogui without needing to add it to each projects.

Here is the repository: https://bintray.com/melmass/MTB

So:

conan remote add MTB https://api.bintray.com/conan/melmass/MTB

You need to use the right package order, here is my conanfile.txt for instance:

[requires]
eigen/3.3.4@conan/stable
nanovg/1.1@melmass/MTB
nanogui/1.1@melmass/MTB

[generators]
cmake
zamazan4ik commented 6 years ago

Here you can read how you can specify dependencies for your library: http://docs.conan.io/en/latest/mastering/conditional.html

p-groarke commented 6 years ago

Oh please support conan. Nanogui dependencies collide if you are using glad yourself or glfw.

svenevs commented 6 years ago

@p-groarke I'm not sure how supporting conan would solve this. See the compilation docs on GLFW. NanoGUI merges the GLFW objects into the built library. I don't know conan well, but if you had something like

[requires]
glfw/version/tag
glad/version/tag

by using NanoGUI, I think it would just replace them with nanogui/version/tag sort of thing.

@ZaMaZaN4iK perhaps you can shed some light on a couple of things to help determine whether or not NanoGUI is capable.

Required NanoGUI Changes

Currently, the NanoGUI installation is really only very useful for getting the python bindings installed. The C++ stuff gets installed, but not all of the features (e.g., third party headers) are installed.

  1. The install scripts do not install the GLFW, NanoVG, or GLAD headers. This can be solved by NanoGUI performing the following header install (with say /usr/local as PREFIX):

    PREFIX/
    include/
        nanogui/
            ... all nanogui headers ...
            third_party/
                GLFW/
                nanovg.h
                etc

  2. Correspondingly, find_package(nanogui) would need to set both PREFIX/include/nanogui and PREFIX/include/nanogui/third_party as include locations. I know how to do both of these, but it'll take a little effort.

  3. We'd need to switch to full target-based "config" style for the installation, particularly for the requisite NANOGUI_GLAD compiler directives. AKA target_compile_definitions. I think this can also be done.

Questions About Conan

I don't know Conan well, but I work with a similar package manager spack. They have some concepts in there that NanoGUI would need.

  1. Since NanoGUI merges GLFW and NanoVG, and statically links against GLAD, is there a way to denote conflicts("glfw") etc -- explicitly specify conflicting packages?
  2. On a similar note, spack has a notion of "providers". This comes in with things like MPI. So the alternative to specifying conflicts would be to make GLFW, NanoVG, and GLAD "virtual" classes, for which NanoGUI provides("glfw") etc.
    • This would be particularly useful for people using NanoGUI, because if you take the conflicts approach then any other projects they use that need GLFW will be disallowed altogether. With the provides interface, the idea would be to specify nanogui.so as the GLFW library to other packages.
  3. NanoGUI vendors Eigen internally to ensure that it can always build on its own -- its designed to build as a submodule rather than require users install it. @melMass mentioned that order is important, how can we know when Eigen is explicitly requested so that we can use that version of Eigen rather than the vendored copy?
  4. Is there anything in Conan with respect to python bindings? If not, we can just disable the python side altogether (which would also be easier).
p-groarke commented 6 years ago

So first questions about conan. I just created a conan dependency for glad, I think the only dependency missing would be Eigen. I'll bug them about it ;) (they have a conan package as well).

  1. You expose your dependencies through conan. GLFW is already available, so you'll say "requires GLFW3.2" for example. I also need GLFW, lets say 3.3, so I do the same. When I include your package, conan automagically fixes the conflict and includes the latest version. Both our projects are linked with 1 and only 1 library.
  2. If I understand correctly what your saying, that is exactly the problem conan fixes. No need to "block" your users ability to use the same libraries. Worst case conan fails and will explain the mismatch version.
  3. Until Eigen is provided on conan, that seems fine. Personally I use cmake ExternalProject_Add for my dependencies that don't have conan packages. So it works cross platform (on Windows as well) and the user doesn't have to do anything. It is better and more flexible than submodules.
  4. I'm not sure about the bindings, however the conan build script is python itself. So you can pretty much do what you want. For example, I am working on a conan dependency that downloads a java jar, then downloads a json swagger API online and generates a c++ project and compiles it. It is very flexible.

There is a cppcon talk about conan, you might want to check them out. From what I understand, you want to enforce your dependency versions to your users. That is problematic to me :)

Good day

svenevs commented 6 years ago

There is a small but very important difference here that is easy to overlook: NanoGUI does not link against GLFW, it "is" GLFW. By "is", the meaning here is that when nanogui.so is built, the objects from GLFW are merged in. This is done to guarantee that NanoGUI can build anywhere from its intended usage: as a submodule. AKA GLFW does not need to be installed by the user. The same happens for NanoVG, and GLAD (when compiled) is linked statically.

The implication: you cannot link against GLFW and NanoGUI in the same project. Doing so will give you duplicate symbols. This is what I am talking about with a "conflicts" (blocking GLFW), or a "provider" interface. The "provider" route requires making changes to the GLFW conan package though, for something that is probably altogether not very common.

For Eigen, it's really just a matter of making sure the requested one is the one included when NanoGUI builds. That's more solvable than the GLFW problem, it's just a cmake configuration.

p-groarke commented 6 years ago

Can't both projects link statically to the same library? I'm pretty sure I've encountered this sort of problem before and haven't had an issue. It's been a while I've used static libraries though. However, you can specify static linking in your conan project. I believe it might link transitively: you link statically to glfw, i need link statically to glfw, I get your glfw symbols.

It is worth experimenting with. If there are conan issues with this sort of scenario, the devs are very friendly and I would bring it up to them.

Is there a reason why you would still link statically to glfw? You mention your users not having to install the library, but that problem is what conan fixes for you. Your users don't have to install anything. All libraries are installed in a predictable location whatever the platform it is. All your users do is

conan install .. --build missing

before running cmake. Thats it. It works :D

edit: They will probably want to use -s build_type=Debug as well. But really there isn't more to it than that.

svenevs commented 6 years ago

Ok, so I think I'm failing at communicating this point. I included too much information. In one sentence: NanoGUI does not link against GLFW, nor NanoVG, the full source of GLFW and NanoVG are compiled directly into NanoGUI. The static linking is only for GLAD.

What it comes down to: NanoGUI is currently self-contained. While having it supported in something like conan would be nice, if changing NanoGUI to be compatible with conan requires that it no longer remains self-contained, that's kind of a deal-breaker.

I went back and spliced in a manually linked GLFW, and you get warnings like this

$  ./mytest
objc[23402]: Class GLFWWindowDelegate is implemented in both /usr/local/opt/glfw/lib/libglfw.3.dylib (0x1028bcb08) and /Users/sven/Desktop/nanogui-test/jericho_clang_build/lib/nanogui/libnanogui.dylib (0x102845448). One of the two will be used. Which one is undefined.
...

Synopsis: I think I misremembered linking errors for this undefined behavior warning. Though things seem to work, it's a terrible idea to accept that as "OK". This can lead to more difficult bugs down the line.

One option would be to enable a CMake bypass for GLFW specifically (also requested in #297). This is kind of the core need to being able to package NanoGUI, in my opinion. Something like -DNANOGUI_SYSTEM_GLFW, which would then resort to finding the system GLFW and if that fails then error the compilation. I don't know how many bypasses of this nature would be required in order to get NanoGUI conan-ready.

I think the two fixes you really need are

  1. Allow an external GLFW to be specified instead of compiling it in manually.
  2. Make NanoGUI install the NanoVG headers under include/nanogui/third_party, and in the header directories specified by CMake, set both include/nanogui and include/nanogui/third_party. That needs to be done to prevent overwriting an existing NanoVG installation.

Basically, I'm happy to answer questions about the build system since I know it pretty well, but I'm not interested in writing the packaging script for something that doesn't need to be packaged.

p-groarke commented 6 years ago

I'll end my part of this discussion mentioning this talk from one of conan's creator : https://www.youtube.com/watch?v=67Am8Z6irjA

I really recommend you watch it, he explains in a much more eloquent way than I ever could why embedding your dependencies is bad. Cheers and good luck with nanogui!

svenevs commented 6 years ago

Cross-ref #347 .

I will be working on updating / enabling external GLFW. Specifically, the GLFW stuff will include CMake updates related to installation as well.

This will pave the way for interested parties to maintain a conan package if they so desire, however I will not be involved in this at least at this time. NanoGUI can be enabled in Conan, but

  1. Conan defaults to static, a GUI library typically should be shared. I'm not sure what the right decision on shared vs static conan option should be default value since most conan packages default to static.
  2. NanoGUI does not have version numbers. You will have to use dates associated with commits (this was advised to me on the #conan cpplang slack channel, which is a great resource if you want to tackle this).

The dependencies for this repository should be

GLAD, even if it has a conan package, should not be done. You should let NanoGUI continue to build that locally. I can explain if you want, but the short version: GLAD is generated from a website with many options / profiles available; the one in NanoGUI is the one that should be used (though we could update GLAD here if needed).

I will post back after the PR that would enable this functionality so that interested parties can communicate / collaborate if desired (ideally somebody commits to maintaining one remote rather than a litany of them).

If you desire NanoGUI // Conan, please comment on anything else that you may need in CMakeLists.txt to enable it. If you do, I can add it to the PR that updates the build system if it makes sense for NanoGUI independent of Conan (e.g., there will be no conan_basic_setup(...) or anything going on, you will have to filter that in as shown in the docs.