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

add osgearth/3.2 #7535

Merged
merged 13 commits into from
Jan 20, 2022
Merged

add osgearth/3.2 #7535

merged 13 commits into from
Jan 20, 2022

Conversation

maspri
Copy link
Contributor

@maspri maspri commented Oct 4, 2021

Specify library name and version: osgearth/3.2

osgEarth is a really cool library, that I am using.

It is on the list in Issue #621.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@maspri
Copy link
Contributor Author

maspri commented Oct 4, 2021

There seems to be no binary package for openscenegraph/3.6.5 + gcc10
I checked, it should build.

@maspri maspri marked this pull request as ready for review October 4, 2021 18:27
@maspri maspri marked this pull request as draft October 4, 2021 18:28
@conan-center-bot

This comment has been minimized.

@stale
Copy link

stale bot commented Nov 6, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 6, 2021
@maspri maspri marked this pull request as ready for review November 8, 2021 09:30
@stale stale bot removed the stale label Nov 8, 2021
@maspri
Copy link
Contributor Author

maspri commented Nov 8, 2021

Sorry I kind of had forgotten about this PR, holidays etc.
I marked as "ready for review" because I need some Input, but there are still some problems:

  • MISSING_DEPENDENCIES for gcc 10 -> see my first comment
  • BUILT + TEST_FAILED for gcc 5 -> How do I debug this, the log seems to only show the build log. Am I missing something?

@maspri
Copy link
Contributor Author

maspri commented Nov 8, 2021

Okay, regarding the second point, of course you can debug with the docker container...
The test failure in gcc5 can be fixed by adding
set(CMAKE_CXX_STANDARD 11)
in the test_package CMakeLists.txt

@conan-center-bot

This comment has been minimized.

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2021

CLA assistant check
All committers have signed the CLA.

define flag in case of msvc static lib
dont build procedural nodekit on windows
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

del draco option for gcc11
@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 removed the infrastructure Waiting on tools or services belonging to the infra label Dec 14, 2021
@conan-center-bot

This comment has been minimized.

@maspri
Copy link
Contributor Author

maspri commented Dec 18, 2021

Okay, so I finally got around to applying your suggestions. I would have never guessed using os_info was the problem, but I can see it makes way more sense to use settings.os.

Finally all green, nice!

@conan-center-bot

This comment has been minimized.

set(GDAL_LIBRARY ${GDAL_LIBRARIES})

# curl
find_package(CURL REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little surprised to see this here.
https://github.com/gwaldron/osgearth/blob/master/CMakeLists.txt
Has this already. I am not familiar with the project but some of this logic seems a tad extra.

Is everything required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

osgearth has custom cmake find modules which unfortunately set different variables than the ones generated by conan (most notably *_LIBRARY instead of *_LIBRARIES, but also different names). These are then used in multiple places throughout, so I opted for setting these non-standard variables in the wrapper.

I agree though, that since I do a patch anyway, the wrapper could probably eliminated. Do you think it is necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapper is needed and I suspect Conan V2 will have an impact...

Let's improve the comments!

 - change license to "LGPL-3.0"
 - remove version range from draco dependency
 - better comment in CMakeLists.txt wrapper
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 18 (8a00af82e3bb3082826ba2a3f1758d1361f542a8):

  • osgearth/3.2@:
    All packages built successfully! (All logs)

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

Successfully merging this pull request may close these issues.

6 participants