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

Magnum hunter #298

Closed
wants to merge 7 commits into
base: master
from

Conversation

4 participants
@pthom
Contributor

pthom commented Dec 3, 2018

Hi,
As discussed, here is the PR for the hunter related modifications.
Cheers

pthom added some commits Nov 30, 2018

Hunter integration: add package/hunter and reference it in main CMake…
…Lists

package/hunter
├── .gitignore               # ignore HunterGate.cmake
├── HunterFetchDeps.cmake    # Fetch SDL, corrade and glfw
├── HunterGate.cmake         # main hunter scripts (ignored in git)
└── HunterInit.cmake         # entry point

@mosra mosra added this to TODO in Project management via automation Dec 3, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Dec 3, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #298   +/-   ##
=======================================
  Coverage   52.89%   52.89%           
=======================================
  Files         323      323           
  Lines       16583    16583           
=======================================
  Hits         8771     8771           
  Misses       7812     7812

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 01a64a6...1af4ef0. Read the comment docs.

@mosra

Thank you!

To save your time, I tried to find answers to my questions by reading Hunter docs. It was overwhelming but I think I got most of the answers from there. I'm still not sure about one thing, see below.

Show resolved Hide resolved CMakeLists.txt
Show resolved Hide resolved package/hunter/HunterInit.cmake
Show resolved Hide resolved package/hunter/HunterInit.cmake Outdated
@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Hi, some updates about hunter CI:

Corrade Hunter CI

We will soon have green lights on corrade in the CI

Magnum Hunter CI

I rebased all my code onto your last commits

@mosra

This comment has been minimized.

Owner

mosra commented Dec 3, 2018

Re the FATAL_ERROR about find_package(EGL): I'm aware of these issues and after reading Hunter docs I'm certain this needs a bigger update on my side. Putting that on my TODO list. I think not having an iOS build in the initial release is okay.

Re Ubuntu 14 vs 16: it's about this, right? https://travis-ci.org/pthom/hunter/jobs/462782227#L1290
I managed to work around this error for GCC builds, but not for Clang builds. This is because std::string move constructor is mistakenly not noexcept in libstdc++ < 5.5 but it's impossible to know what libstdc++ version is actually used.

I'll be switching to Ubuntu 16 soon anyway, so unless this is a critical issue for Hunter, I'm not sure we should bother too much with it.

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Re the FATAL_ERROR about find_package(EGL): I think not having an iOS build in the initial release is okay.

I think too !

Re Ubuntu 14 vs 16: it's about this, right? https://travis-ci.org/pthom/hunter/jobs/462782227#L1290
I managed to work around this error for GCC builds, but not for Clang builds. This is because std::string move constructor is mistakenly not noexcept in libstdc++ < 5.5 but it's impossible to know what libstdc++ version is actually used.

Exactly, the error message is strange, given that the corrade code seems to do what is appropriate

I'll be switching to Ubuntu 16 soon anyway, so unless this is a critical issue for Hunter, I'm not sure we should bother too much with it.

I hope the maintainer of hunter will bare with me for I changed from travis to xenial. Having a package like magnum is probably worth it !

@mosra

This comment has been minimized.

Owner

mosra commented Dec 3, 2018

I just pushed mosra/corrade@c09a22e which is hopefully a better workaround for the noexcept issue and could make build on 14.04 possible again. Not on master yet, waiting for the build to succeed but looks good so far. Could you try on the CI with this commit and 14.04?

EDIT: it's on master now.

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Hum, it would be too long with travis.
However I have good news, with a test from docker :

If I use ubuntu 14.04

diff --git a/Docker/ubuntu/Dockerfile b/Docker/ubuntu/Dockerfile
index abc754c..eecd979 100644
--- a/Docker/ubuntu/Dockerfile
+++ b/Docker/ubuntu/Dockerfile
@@ -1,7 +1,7 @@
-FROM ubuntu:16.04
+FROM ubuntu:14.04

And then inside the docker container I try to build corrade I have the error:

> ./TLDR_hunter.py test-build corrade gcc-7-cxx14
/sources_docker/corrade/src/Corrade/Utility/FileWatcher.cpp:59:14: error: function ‘Corrade::Utility::FileWatcher& Corrade::Utility::FileWatcher::operator=(Corrade::Utility::FileWatcher&&)’ defaulted on its redeclaration with an exception-specification that differs from the implicit exception-specification ‘’
 FileWatcher& FileWatcher::operator=(FileWatcher&&)

then I fetch your modif

> cd corrade
> git pull upstream ae1fc17d3d77b1bec248382fbdbefd8b9fbb44a9

And inside the container, it works !

> ./TLDR_hunter.py test-build corrade gcc-7-cxx14
SUCCESS

What is your opinion: do you prefer that I reverse the travis builds to travis ?

@mosra

This comment has been minimized.

Owner

mosra commented Dec 3, 2018

Awesome! :) It's on master now.

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Here is my checklist in order to finish this.

What do you think of it? I will need your input concerning the releases url and sha1 :-)

Corrade hunter package / v2018.10 => DONE: see ruslo/hunter#1646


Corrade hunter package / v2018.12

  • Create a release on corrade main repo
  • Add the release link and it's sha1 to hunter / make it the second and default version
  • Push PR to hunter

Magnum hunter package

Magnum (optional steps)

  • Integrate example_apps into magnum-bootstrap ?
@mosra

This comment has been minimized.

Owner

mosra commented Dec 3, 2018

Hunter CI Back to travis ?

Not really sure what this means, sorry 😅

Create a release

I'm planning to have a 2018.12 release, but that will happen later this month (around Dec 30th I think) as I still have quite a few important features I want to finish before tagging another version. Would it be possible to use a temporary tag on your fork in the meantime? Once 2018.12 is out, I can switch the packages to the "upstream" tag.

Integrate example_apps into magnum-bootstrap ?

Yes, definitely. I can do that once Magnum is available in Hunter, I also wanted to play with it a bit to get a feeling how it's used. For the rest of the magnum_hunter I'm not quite sure yet, maybe it could go also to package/hunter/ to aid with packaging and testing in the future. Would it be okay to just keep it on your account until I have a clearer idea?

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Hunter CI Back to travis ?

I need to discuss this directly with Ruslan Baratov (maintainer of hunter)

Create a release: I'm planning to have a 2018.12 release, but that will happen later this month (around Dec 30th I think) as I still have quite a few important features I want to finish before tagging another version

There is no hurry.

Would it be possible to use a temporary tag on your fork in the meantime?

Not on my fork. If we want to publish a release before the 2018.12 release, I would need to place my fork inside https://github.com/hunter-packages/ (which I did not).
But if I copy the fork there, it should be ok, and I can create a tag there.

For the rest of the magnum_hunter I'm not quite sure yet, maybe it could go also to package/hunter/ to aid with packaging and testing in the future. Would it be okay to just keep it on your account until I have a clearer idea?

Of course, this is the good way to go.

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

One question that is abolutely not related.
I am quite new to magnum actually. When I build the primitive example on my mac (https://github.com/mosra/magnum-examples/tree/master/src/primitives) , I get a black screen with the following output:

Platform::Sdl2Application: warning: the executable is not a HiDPI-enabled app bundle
Renderer: AMD Radeon Pro 560 OpenGL Engine by ATI Technologies Inc.
OpenGL version: 4.1 ATI-2.2.8
Using optional features:
    GL_ARB_ES2_compatibility
    GL_ARB_separate_shader_objects
    GL_ARB_texture_filter_anisotropic
    GL_ARB_texture_storage
    GL_ARB_vertex_array_object
    GL_EXT_debug_label
    GL_EXT_debug_marker
Using driver workarounds:
    no-layout-qualifiers-on-old-glsl

Did I miss a setting?

It does work on windows, though.

@mosra

This comment has been minimized.

Owner

mosra commented Dec 3, 2018

No, you didn't. it's a known bug in macOS 10.14. Shake the window, minimize/maximize it or click into it to make the contents appear. Related info for SDL is here: https://discourse.libsdl.org/t/macos-10-14-mojave-issues/25060 (so far I'm not aware of a new version fixing this :/ ...), more info (and some workarounds) here in a GLFW issue: glfw/glfw#1334

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Shake the window, minimize/maximize it or click into it to make the contents appear

This is the funniest workaround I have heard off :-)

It does not work on my side because I cannot resize the example app window. It is not important at all, though: I can work on linux / windows.

@mosra

This comment has been minimized.

Owner

mosra commented Dec 3, 2018

Yes, it's quite annoying. Not related to Magnum at all, but rather to how the underlying toolkits (SDL/GLFW) manage the GL context. I hope the fixed versions of SDL / GLFW get released soon.

For the release tag: so is it okay to have the release delayed until I get 2018.12 out? I really don't want to be blocking you from getting your work done :)

Oh, but Corrade (2018.10) could get packaged as-is already so we can tick at least some boxes, right? Then, once that's done, the hunter URL here could get updated to an upstream version of Hunter that contains Corrade and then this PR could get merged without any TODOs left. Is that a reasonable path forward?

@pthom pthom referenced this pull request Dec 3, 2018

Open

Add package magnum #1635

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 3, 2018

Ok, let's try to tick some boxes for corrade :

Here is the first PR out of two : ingenue/hunter#300
(this one contains the CI scripts)

Show resolved Hide resolved CMakeLists.txt
Show resolved Hide resolved CMakeLists.txt
Show resolved Hide resolved package/hunter/HunterInit.cmake Outdated

@pthom pthom force-pushed the pthom:magnum_hunter branch from 43842a0 to 1af4ef0 Dec 3, 2018

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 5, 2018

Hi,
I just pushed some modifs to ingenue/hunter#300 (review)

This version should pass, as it disables the hack for corrade-rc.

For the future release of corrade on hunter, we have two options :

  • Leave the p.pkg.corrade branch as it is, i.e do not build iOS and Android packages in hunter's CI (I added a note inside the readme that explains it can work if corrade-rc is in the path)
  • Add some scripts into the corrade repo : something that will cause corrade-rc to try to build automatically if find_program(corrade-rc) fails (this might be based on a cmake flag) . I do not think it is worth adding a dependency to hunter for this, as corrade does not require hunter (it has no dependency), so that a shell script called by cmake could be sufficient.

Do you have a preference?

@mosra

This comment has been minimized.

Owner

mosra commented Dec 5, 2018

@pthom thank you!

I would leave it as-is, for now. With other cross-compilation packages I'm usually adding a dependency to native Corrade installation (so e.g. ArchLinux emscripten-corrade depends on native corrade). Not sure if anything like that can be expressed in Hunter, but again, let's leave it for a later time :)

@ruslo

This comment has been minimized.

ruslo commented Dec 5, 2018

I do not think it is worth adding a dependency to hunter for this, as corrade does not require hunter (it has no dependency), so that a shell script called by cmake could be sufficient

Target version of package depends on host build of same package. So there is a dependency.

Not sure if anything like that can be expressed in Hunter

Yes, it can be, see comments:

@mosra

This comment has been minimized.

Owner

mosra commented Dec 5, 2018

@ruslo right, thanks, sorry that you had to repeat everything again 😅

Yes, now I think I know how this could be done (as a next step after the native packages are done). Since I still had some problems with e.g. find_package() not having REQUIRED clauses for Android / iOS and so I need to fix that first.

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 5, 2018

corrade is merged into hunter !
ruslo/hunter#1646

@mosra

This comment has been minimized.

Owner

mosra commented Dec 5, 2018

Great!

I'll reference it in the building documentation. Is there anything I should mention besides "Corrade is available in Hunter" and linking to https://docs.hunter.sh/en/latest/packages/pkg/corrade.html ?

Can you update the Hunter release URL here? I'll merge this PR right after.

@ruslo

This comment has been minimized.

ruslo commented Dec 5, 2018

I'll reference it in the building documentation. Is there anything I should mention besides "Corrade is available in Hunter" and linking to https://docs.hunter.sh/en/latest/packages/pkg/corrade.html ?

I think just link should be enough, user have to read basic Hunter documentation anyway. By the way you can add it as a badge: https://docs.hunter.sh/en/latest/creating-new/create/cmake.html#badge

@mosra

This comment has been minimized.

Owner

mosra commented Dec 5, 2018

Yay, badge! Of course :)

Done in mosra/corrade@a4c4fe6, updated docs here.

@mosra mosra moved this from TODO to In progress in Project management Dec 5, 2018

@mosra mosra added this to the 2018.1d milestone Dec 11, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Dec 11, 2018

Merged a squashed version in 91b41a7, I did a bit of simplification and coding style updates on top.

@pthom once I tag 2018.12, I'll let you know, and in case you are not around, I'll attempt to finish the Magnum package submit myself. Thanks a lot for the contribution! 👍

@mosra mosra closed this Dec 11, 2018

Project management automation moved this from In progress to Done Dec 11, 2018

@pthom

This comment has been minimized.

Contributor

pthom commented Dec 12, 2018

@mosra : thanks for the update!

Please do contact me when you release the version, I'll try to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment