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

Conan package #304

Closed
helmesjo opened this issue Jan 5, 2019 · 85 comments
Closed

Conan package #304

helmesjo opened this issue Jan 5, 2019 · 85 comments
Milestone

Comments

@helmesjo
Copy link

helmesjo commented Jan 5, 2019

I'm writing a conan recipe for Magnum (already got one for Corrade successfully building). I've wrestled a few days now with these issues (each try takes a lot of time) so I'd thought I'd look for some help/ideas here!

Issue

I'm building conan packages for GCC, Clang (etc.) inside docker containers.
In the conan recipe, prior to building magnum, it installs libgl1-mesa-dev (or libgl1-mesa-dev:i386 respectively) on the host. But when later building, it either:

  1. Fails to find OpenGL libs when targeting x86 (for all builds on Ubuntu) if only installing libgl1-mesa-dev:i386:
    CMake Error at /usr/share/cmake-3.12/Modules/FindPackageHandleStandardArgs.cmake:137 
    (message):
        Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY OPENGL_glx_LIBRARY)
    
    Above can be fixed* (for all but Ubuntu Trusty) by installing both libgl1-mesa-dev & libgl1-mesa-dev:i386 (in that order, else 1. happens again), but then it:
  2. fails to link OpenGL (error adding symbols: File in wrong format) on Ubuntu Bionic & Cosmic when targeting x86 (it appears link the 64bit lib).

*There are a lot of issues with multiple mesa-dev packages on the same host.

Details:

  • CMake 3.12
  • Ubuntu x64: Trusty (GCC 4.9), Xenial (GCC 5), Artful (GCC 6 & 7, Clang 3.9. 4, 5), Bionic (GCC 8, Clang 6), Cosmic (Clang 7) (using the conan docker images)

So, any ideas? Anyone seen similar issues? Am I missing something obvious?

@helmesjo helmesjo changed the title [Help Needed]: Issues cross building to x86 on x86_64 Ubuntu host [Help Needed]: OpenGL issues when cross building to x86 on x86_64 Ubuntu host Jan 5, 2019
@helmesjo helmesjo changed the title [Help Needed]: OpenGL issues when cross building to x86 on x86_64 Ubuntu host OpenGL issues when cross building to x86 on x86_64 Ubuntu host Jan 5, 2019
@mosra
Copy link
Owner

mosra commented Jan 5, 2019

Yay, Conan! 😍 There's an in-progress Hunter package as well (#298), so with this it's complete :)

If I see correctly, this happens only when cross-compiling from 64b to 32b, right? In my experience, building natively on either 32b or 64b never had any issues (in particular, I have a Travis CI build and it also needed just libgl1-mesa-dev).

I wanted to suggest looking at how other CMake-based packages handle this, but seeing your other open issues (bincrafters/community#602 / bincrafters/community#603), it seems you already tried that. Other ideas:

  • (I have no idea how Conan works, but) is it possible to supply location of these libraries explicitly? That's the final solution when everything else fails and always did the job for me. Magnum doesn't need the headers, just the libs.
  • FindOpenGL in CMake 3.10 switched to preferring GLVND, if found, and I'm opting-in for that. The error message looks like it tried to find GLVND (so libOpenGL.so + libGLX.so instead of libGL.so) and failed. I have no idea what Ubuntu ships, it's possible that the older versions don't supply these libraries at all. However, in that case FindOpenGL should fall back to the old thing if I understood correctly. Maybe something related to the cross-compilation where FindOpenGL assumes something it shouldn't? Maybe the compat 32b libraries don't ship the GLVND things?

@helmesjo
Copy link
Author

helmesjo commented Jan 5, 2019

Yeah, conan is a dream come true (once the packages are in place)! 😇

From what I can tell (I've been staring at this too hard recently, so my brain is pretty cooked), yes the issue is only related to cross building from a x64 host to x86. Building natively is probably a sane path to go considering all issues around OpenGL libs in general (been seeing a lot in my google quest). I'll probably try this first.

  1. Yes, conan recipes are just the glue for downloading, building & packaging software, and written in python so patching/fixing up of any sort is rather "trivial". For example, find_package allows "hints" of where it should look first. I could replace all find_package related to OpenGL with an equivalent that also specifies the path. Then it's just a matter of reliably obtaining the correct path (shouldn't be too hard in linux I assume).

  2. Yeah, something is probably off with the FindOpenGL-script regardless. It should be "target arch"-aware and select the correct one... But those scripts are all just "naive brute force locators", so I guess one can't expect too much magic... 🎆

Thanks for the feedback! I'll keep digging!

@mosra
Copy link
Owner

mosra commented Jan 5, 2019

I think it should be possible to dig the problem out of the FindOpenGL script and fix it (after all, the core of the problem is just a file not being found in this particular crosscompilation setup), but then:

  • if I submit a PR to CMake, it'll be a year or so until it gets integrated & released, not to mention conan will probably not jump to latest and greatest CMake right away
  • I could maintain the forked/fixed version inside magnum, but this stuff is so complex right now given GLVND and all that stuff that I'd rather rely on CMake devs having the responsibility

I think the more straightforward way would be to just build natively on 32bit. Besides that, didn't Ubuntu ditch 32bit support for the latest release already (19.04)? I heard something related to that.

@helmesjo
Copy link
Author

helmesjo commented Jan 5, 2019

I agree, and did just that! Now we'll just have to wait and see if this goes from red to green! 😊
Build Status Build Status AppVeyor

@Croydon
Copy link

Croydon commented Jan 5, 2019

Hi @mosra!
Nice to see that you are interested in Conan as well!
Would you be interested in providing official support for Conan once the recipe is stable?
We can help you to setup everything which is required if you want 😄

Otherwise, I believe @helmesjo is going to include the recipes in @bincrafters and we are maintaining it there, but obviously official support is always nicer 😄

@helmesjo
Copy link
Author

helmesjo commented Jan 5, 2019

Alright, so native builds did the trick. Closing this for now!

@helmesjo helmesjo closed this as completed Jan 5, 2019
@mosra
Copy link
Owner

mosra commented Jan 6, 2019

@helmesjo yay!

@Croydon I'm afraid I don't have the capacity to provide official support (as in maintaining them myself), I'm already offloading Vcpkg and Debian package support to people who are using those platforms directly -- because they can spot the issues much easier. What I can do, however, (and what I'm usually doing), is reviewing them to ensure they provide all desired options and features (and also pinging you in case a new version is about to get released).

@Croydon
Copy link

Croydon commented Jan 6, 2019

You could still ping us at any time when either CI or users are complaining about problems, so it would be more reviewing then maintaining by yourself. We (as in Bincrafters) kinda have this relationship with the maintainer of Flatbuffers, as he doesn't know much about Conan as well: https://github.com/google/flatbuffers/commits/master/conanfile.py

But if you don't feel comfortable with this is also fine and we will go ahead at @bincrafters 😄

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

Right, so what the stuff I need to do for this semi-official support? :) Looking at flatbuffers, I see just conanfile.py there + a large build matrix in .travis.yml, while https://github.com/helmesjo/conan-magnum has many more files. If it would be about just maintaining conanfile.py directly in this repository, I'm all for it. This also means if there's a problem, it doesn't depend just on PRs to @helmesjo's repository, but anybody can just pick it up from the main magnum repo and fix it.

But ... the problem I see here is that I already have quite a huge build matrix (see here: https://magnum.graphics/build-status/) and adding a dozen of other jobs just for Conan will make it unusable, since the builds would take forever (it's already bad enough with AppVeyor taking 2+ hours). Is there a way around that?

@Croydon
Copy link

Croydon commented Jan 6, 2019

The amount of files is depending on the question if we would use your CI or another one. If it is only about minimal Conan support without CI support, then this would be conanfile.py + test_package https://github.com/helmesjo/conan-magnum/tree/stable/2018.10/test_package

All of the other files are only needed to get the CI + publishing going (see https://github.com/bincrafters/conan-templates-upstream as a reference)

We could maintain conanfile.py + test_package (= 4 files in total) here and use our Bincrafters CI and repository to publish the build packages

@helmesjo
Copy link
Author

helmesjo commented Jan 6, 2019

Just the last piece of the puzzle... (semi-related to this issue so I hope it's ok to ask here 😇 ):

I fixed the link order, and also added OpenGL to the link list (all of this is applied to packages depending on Magnum), and in the process added a line in the test package that verifies all is linked correctly (this is like a mini-package depending on Magnum & build+run by conan after building Magnum, just for verification).
This worked dandy for all but MacOS, which just anonymously fails when executing the test package...

@mosra Do you know of anything else that is required on Mac? A package that needs to be installed? Some other framework that needs to be linked/added for Mac? Could this be a Travis+Mac thing?

@mosra mosra changed the title OpenGL issues when cross building to x86 on x86_64 Ubuntu host Conan package Jan 6, 2019
@mosra mosra reopened this Jan 6, 2019
@Croydon
Copy link

Croydon commented Jan 6, 2019

@helmesjo I quickly deleted my message(😁) as it didn't see related on a second thought, even more since this is the case on all configurations but only macOS fails.

@helmesjo
Copy link
Author

helmesjo commented Jan 6, 2019

Haha np. Yeah also that target is optional (unless building tests). But I changed Corrade to build with_testsuite=True nonetheless, to be consistent with the main repo.

@Croydon
Copy link

Croydon commented Jan 6, 2019

Is this testsuite there to test magnum/corrade itself or applications using it? In the first case it should stay disabled

@helmesjo
Copy link
Author

helmesjo commented Jan 6, 2019

It's for magnum & corrade from what I can tell, so yes it adds unnecessary build time for sure. You know what, I'll set it to false again! 😄

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

uh oh, slow down, i am unable to type so fast 😅

I hope it's ok to ask here 😇

Sure, of course ;) since it's now mainly about Conan, I'm renaming and reopening.

link order

different default value

There's Using Magnum with custom buildsystems in the documentation, listing link order and all main packages of magnum and corrade. All of them should be present in the options (IIRC Trade and DebugTools were missing) and those which are deprecated (Shapes and GlutApplication) don't have to be there at all, since I will be removing them soon anyway. The default values should match the defaults from option() calls in root CMakeLists.txt, except maybe for Sdl2Application, which is good to have by default.

Not sure if there's something other from the option()s that's missing in the package options, tho.

Is this testsuite there to test magnum/corrade itself or applications using it?

It's a whole test suite framework (see the docs) and I'm encouraging the library users to use it as well, since it's well integrated with the lib and has some features others don't. But apps can be tested by anything else as well, so it's completely optional (but enabled by default, yes). Building of tests is controlled by BUILD_TESTS, BUILD_GL_TESTS and BUILD_AL_TESTS CMake options and I don't think enabling these makes any sense, nor including them in the package options.

added OpenGL to the link list

Hmm. For some reason I thought Conan is CMake-based and this all would be handled transitively by CMake. Isn't that the case?

If not, then PluginManager, Sdl2Application and GlutApplication needs to link to libdl on Linux, and then, in case you build the Audio library as well, it depends on OpenAL. This should be all I think, there are some extra deps for iOS and Android, but you don't handle those platforms yet, right?

For OpenGL there's an ability to use GLES (TARGET_GLES etc.), in which case it doesn't link to libGL on Linux but to libGLES. That's mainly for platforms like RPi. Not sure how to handle that (or if Conan supports such platforms).

The amount of file is depending on the question if we would use your CI or another one.

Since that would mean blowing up the job count, I think this would need to be on your CI, then. For all other packages I'm having stuff inside the package/ directory, so there could be package/conan/ containing the conanfile.py and the test cmake + cpp file. Would that help with anything?

@helmesjo
Copy link
Author

helmesjo commented Jan 6, 2019

Ah sweet, that was the documentation I was thinking about but never actually searched for... 😊
I'll look into this!

@Croydon
Copy link

Croydon commented Jan 6, 2019

It's a whole test suite framework (see the docs) and I'm encouraging the library users to use it as well, since it's well integrated with the lib and has some features others don't. But apps can be tested by anything else as well, so it's completely optional (but enabled by default, yes).

Alright then, sorry @helmesjo, maybe your first instinct was right and True should be the default value. Maybe we need to slow down a bit :)

Hmm. For some reason I thought Conan is CMake-based and this all would be handled transitively by CMake. Isn't that the case?

Conan can work together with absolutely all build systems. However, for the common ones it ships a lot of helper tools to deal with common scenarios and glue together the Conan world with the build system's one. For CMake Conan can in theory detect the libraries automatically, however, only with some limitations. For example, it can't detect the right order to link stuff and sometimes the order is important. Also it might not be perfect and misses libraries out. For these reasons recipes who want to be included in the official conan-center should list the libraries manually.

To understand this a little bit better: Even Magnum is using CMake, consumers of Magnum might not, so Conan needs to translate all of these information between different building systems. So even consumers might use Meson it still links all libraries in the correct order.

This should be all I think, there are some extra deps for iOS and Android, but you don't handle those platforms yet, right?

Conan can handle those platforms, but typically we don't deliver for this platforms. At least not right now, who knows what the future brings :)

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

For these reasons recipes who want to be included in the official conan-center should list the libraries manually.

Yeah, now I get you completely, thanks.

So, looking in FindMagnum.cmake and FindCorrade.cmake should help you getting the various dependencies. Just the highlighted portion and the various if(_component STREQUAL something) inside are important. As a rule the project itself has no external dependencies but the components (might) have. I don't remember the particulars from the top of my head (especially related to X11-related things, GL and Vulkan), so that's why I'm delegating you there. Sorry about this not being clearly documented yet, it's on my TODO list, but the TODO list has also hundreds of other items 😉

but typically we don't deliver for this platforms

Sure, it's easy to enable these platforms later, if someone would need them.

@mosra mosra added this to the 2019.01 milestone Jan 6, 2019
@Croydon
Copy link

Croydon commented Jan 6, 2019

Since that would mean blowing up the job count, I think this would need to be on your CI, then.

This is fine, we can use Bincrafters' CI 👍

For all other packages I'm having stuff inside the package/ directory, so there could be package/conan/ containing the conanfile.py and the test cmake + cpp file. Would that help with anything?

The test_package directory can be anywhere you want. I'm not 100% sure about the conanfile.py, however. It should work to have it in package/, when we are doing it like in the hunter package: The recipe itself is downloading the source again from GitHub. This is a small sidestep from our common practice to have what we call in-source recipes when we have official recipes, but it should be fine. In-source recipes are making direct use of the projects source file around the recipe file and doesn't need to download them again from somewhere. This also allows user to check out a random git commit and still being able to install it via Conan. If we write it like an out-of-source recipe it can only install the release version it has hard coded in it. Of course, this is only relevant for people directly installing it from the git repository. When installing from a Conan repository you just pick the release version anyway via the reference,
e.g. conan install corrade/2018.10@mosra/stable

@Croydon
Copy link

Croydon commented Jan 6, 2019

To do for Corrade (Magnum can be done afterwards much more easily)

  • Getting Corrade working at https://github.com/helmesjo/conan-corrade
    • I'm leaving it to @helmesjo to decide when this is (initially) "done"
  • Creating a Conan org + repository on Bintray for your Conan recipes, e.g. magnum/public-conan
    • In order to get the recipes eventually into conan-center, we need to upload it to a Conan repository on Bintray. In theory we could of course use our Bincrafters one but this would be rather uncommon giving the fact that you are official approving the recipes. So I would strongly advocate that you are getting a free account on Bintray, creating a Bintray organisation with a name like magnum (or any other name you are comfortable with) and are adding the bincrafters-user account which is acting as our upload bot. This would also make it look more official + we can work together seemlessly, but still keep it technically separate. If you should ever decide to use another CI system, you can do that without any interruption for users.
  • Creating pull request for Corrade which is adding the conanfily.py + CMakeLists.txt + test_package in package/conan + it should probably get a README.md to explain how to use Conan
  • @Croydon: Setting up the CI scripts in https://github.com/bincrafters/conan-corrade + adding @helmesjo & @mosra as collaborators

@Croydon
Copy link

Croydon commented Apr 5, 2019

Continuing the conversation over here...

Some Conan recipes like Magnum do require some system_requirements. Over at Bincrafters we do have currently a little confusion about mapping all ARM architectures to their correct Debian/Ubuntu/Fedora etc. (or better apt/yum) package suffix. Cross-referencing: bincrafters/community#704

The Magnum recipe seems to be almost ready, but that's one of the things we should figure out first.

https://github.com/Croydon/magnum/blob/533f9361c4c0fec45896e4d30082aa86cd8d04b4/conanfile.py#L129

@mosra
Copy link
Owner

mosra commented Apr 7, 2019

Vaguely related to the above -- the gl/gles branching should be like this instead (TARGET_GL is a superset of TARGET_GLES, so if GLES is selected, GL is as well):

                if self.options.target_gles:
                    packages.append("libgles1-mesa-dev")
                elif self.options.target_gl:
                    packages.append("libgl1-mesa-dev")

Oh -- and then, also, for Vulkan -- which means there's a new target_vk option that I forgot about (sorry!):

                if self.options.target_vk:
                    packages.append("libvulkan-dev")

@Croydon
Copy link

Croydon commented Apr 16, 2019

@mosra Would it be valid to actually enable more than one of the target_ options at the same time?

@Croydon

This comment has been minimized.

@Croydon
Copy link

Croydon commented Apr 16, 2019

Added target_vk: Croydon@2a999b9

Added OpenAL depenceny for with_audio: Croydon@7420219 (inexorgame-obsolete/entity-system-inactive#113)


I'm seeing that there is also a target_gles2 which isn't handled so far, we should add it as well?

@mosra
Copy link
Owner

mosra commented Apr 20, 2019

Sorry about the delayed reply.

Actually, it's like this (a bit complex, I must admit):

  • if WITH_GL is enabled (this option seems to be missing as well?), the Magnum::GL library is built and that one depends on some GL library:
    • if TARGET_GLES is enabled, then it's libGLES,
    • otherwise, libGL is used (so the order of the conditions I used above is correct).
  • similarly, if WITH_VK is enabled, the Magnum::Vk library is built

The TARGET_GL and TARGET_VK options control if libraries other than Magnum::GL and Magnum::Vk are integrated with OpenGL / Vulkan -- for example, if Platform::Sdl2Application is able to create OpenGL contexts or not. In particular, having WITH_GL enabled but TARGET_GL disabled still means the library depends on libGL / libGLES. In other words, TARGET_GL / TARGET_VK don't add any dependencies, WITH_GL / WITH_VK do.

If TARGET_GLES is enabled, the TARGET_GLES2 controls whether the library targets OpenGL ES 2.0 or OpenGL ES 3.0+. The APIs have quite significant differences (there's a lot that's either in 2 or in 3 but not in both) and so it made sense to have this a compile-time option.

@mosra mosra mentioned this issue Jun 6, 2019
60 tasks
@helmesjo
Copy link
Author

Just a friendly ping:
How are things looking @Croydon ? Sorry for dumping this on you, I've been (and still am) focusing on very different things atm!

@Croydon
Copy link

Croydon commented Jun 18, 2019

There are still several options missing etc. see the last few comments

I once again have to concentrate on exams and related stuff, so I won't get this done in the next few weeks

However, I haven't forgotten about Magnum, so I will do this at some point or another if nobody else is faster 😄

@mosra
Copy link
Owner

mosra commented Jul 16, 2019

Ping -- any chance we could wrap this up soon?

@mosra mosra modified the milestones: 2019.0b, 2019.0c Aug 27, 2019
@Croydon
Copy link

Croydon commented Oct 4, 2019

@mosra I just wrote you on Gitter. Sorry for the long silence once more 🙈

@Croydon
Copy link

Croydon commented Jan 14, 2020

Status update:

Corrade is now in the Conan Center Index (CCI) and probably needs some fixes and fine tuning going forward (specifically with the CMake scripts).

I'm still interested in pushing Magnum into CCI as well

We raised the quality standards and don't accept system_requirements() anymore (with few very, very low-level exceptions; for many reason, a good summary is written here)

Which means that we need to do a lot of other ground work before stuff like Magnum can be finally included. We are on it, but it is hard to say when we are done with it.

To mature the Magnum recipe before it is ready for CCI I will probably add it to Bincrafters as an intermediate solution (we don't need to be that strict there with quality standards as in CCI, so e.g. system_requirements() are fine for the time being).

I would recommend to close this issue for now. With the introduction of CCI and the change of workflows most work won't happen here in this repository anyway. I'm not an user for Magnum (currently) so I lack a lot of knowledge about it and might appreciate some advises while packaging it.

We didn't have any conclusion yet if upstream repositories should contain the Conan recipes too (summary: advantages for people working with/developing the git version, but obvious downsides for maintaining them as it duplicates the recipe; conan-io/conan-center-index#22). If we have a stable Magnum recipe in the future and we (Conan people + Magnum) come to the conclusion that it is worth of having it in the Magnum repository directly we can create a new issue for it.

@mosra
Copy link
Owner

mosra commented Feb 12, 2020

Thank you for the continued effort on this 👍

One thing that I should probably do before closing the issue is properly documenting that Corrade is in CCI. Right now, the docs say this -- since I'm not a Conan user, I don't know what piece of information is needed and what could be thrown away. Could you write up a short paragraph / code snippet etc that I should put there instead? Thanks a lot!

We didn't have any conclusion yet if upstream repositories should contain the Conan recipes too

So, I'll keep the conanfile.py in the corrade repo, trying to keep it up-to-date for Git users, and if we ever decide it's not worth to maintain a duplicate anymore, I can remove it.

I'm not an user for Magnum (currently)

... and I'm not an user for Conan (currently) :) Which means, I'm not monitoring the CCI repo, so if there's some discussion related to the magnum packages and you think I should be involved, please CC me there. I'll try to help if I can.

@mosra mosra modified the milestones: 2020.0a, 2020.0b Jun 24, 2020
@rb123454321
Copy link

@helmesjo New release, mosra asked me to ping you to update the Conan package.

@helmesjo
Copy link
Author

@mosra @RokoBurilo Ahh very nice, thanks! :D

@mosra
Copy link
Owner

mosra commented Jun 30, 2020

FYI I made sure to update the version in corrade's conanfile.py before tagging v2020.06, tho not sure if that file is still relevant :)

@Croydon
Copy link

Croydon commented Jul 3, 2020

Issue conan-io/conan-center-index#1397 seems to be soon fixed via conan-io/conan-center-index#2111

And @helmesjo works on adding the new version here: conan-io/conan-center-index#2109

FYI I made sure to update the version in corrade's conanfile.py before tagging v2020.06, tho not sure if that file is still relevant :)

Depends if we still want to support git-versions / arbitrary git commits with a Conan recipe. Maybe doubling maintenance cost for this advantage is just not worth it. If this would be the decision you could remove all Conan related files from your repositories and everything would be in conan-center-index

But there is still no overall census in this regard... conan-io/conan-center-index#22

@helmesjo
Copy link
Author

Just noticed that a package has been added to conan-center.

@jgsogo
Copy link

jgsogo commented Sep 25, 2021

I have also prepared a PR for magnum-plugins and I have another prepared for magnum-integration 💪

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

No branches or pull requests

6 participants