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

libosmium: new recipe #23720

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

libosmium: new recipe #23720

wants to merge 5 commits into from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Apr 23, 2024

Adds libosmium: https://github.com/osmcode/libosmium

Fast and flexible C++ library for working with OpenStreetMap data.

Requires #23829 for the gdal option to be able to be set to True

Packaging status

@conan-center-bot

This comment has been minimized.

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks! Can't approve because I ran out of time for a proper review for the day, but from a glance looks good :)


int main() {
try {
const osmium::io::File input_file{"missing_file.pbf"};
Copy link
Member

Choose a reason for hiding this comment

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

Nice idea with passing a missing file, very on topic with the simplification of the test packages :)

@AbrilRBS AbrilRBS self-assigned this Apr 23, 2024
@valgur valgur mentioned this pull request Apr 23, 2024
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks! I see a minor issue with zlib, other than that, I have checked that this properly links to gdal locally too, so I think it's good to go :)

recipes/libosmium/all/conanfile.py Outdated Show resolved Hide resolved
@valgur valgur requested a review from AbrilRBS April 30, 2024 10:39
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Extra question: is the gdalcpp vendorizen library something we should worry about?

@conan-center-bot

This comment has been minimized.

@valgur valgur mentioned this pull request Apr 30, 2024
@valgur
Copy link
Contributor Author

valgur commented Apr 30, 2024

@RubenRBS Thanks for pointing it out. I did not notice that it's a vendored external library. I turned it into a separate package: #23829.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 4 (887e2e23f1e8351b9c94d2d5c0e7db57aaa83229):

  • libosmium/2.20.0:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 4 (887e2e23f1e8351b9c94d2d5c0e7db57aaa83229):

  • libosmium/2.20.0:
    All packages built successfully! (All logs)

@nickblock
Copy link

any chance we can get this merged?

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

When trying to compile with all options set to false, I get this in the test_package:

-- Configuring done (0.8s)
-- Generating done (0.0s)
-- Build files have been written to: /Users/abril/coding/conan-center-index/recipes/libosmium/all/test_package/build/apple-clang-16-armv8-gnu17-release

libosmium/2.20.0 (test package): Running CMake.build()
libosmium/2.20.0 (test package): RUN: cmake --build "/Users/abril/coding/conan-center-index/recipes/libosmium/all/test_package/build/apple-clang-16-armv8-gnu17-release" -- -j12
[ 50%] Building CXX object CMakeFiles/test_package.dir/test_package.cpp.o
In file included from /Users/abril/coding/conan-center-index/recipes/libosmium/all/test_package/test_package.cpp:1:
In file included from /Users/abril/.conan2/p/b/libos33eefa4f13402/p/include/osmium/io/any_input.hpp:48:
In file included from /Users/abril/.conan2/p/b/libos33eefa4f13402/p/include/osmium/io/o5m_input.hpp:42:
/Users/abril/.conan2/p/b/libos33eefa4f13402/p/include/osmium/io/detail/o5m_input_format.hpp:56:10: fatal error: 'protozero/exception.hpp' file not found
   56 | #include <protozero/exception.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/test_package.dir/test_package.cpp.o] Error 1
make[1]: *** [CMakeFiles/test_package.dir/all] Error 2
make: *** [all] Error 2

ERROR: libosmium/2.20.0 (test package): Error in build() method, line 22
	cmake.build()
	ConanException: Error 2 while executing

which seems to imply that either pbf is non-optional, or that its presence is not properly propagated

@valgur
Copy link
Contributor Author

valgur commented Nov 4, 2024

Thank you for the extra checks. 🙂

I was using functionality from the optional io component in the library, which requires protozero.
I simplified test_package to only test against objects exposed by the main header as a fix.

@valgur
Copy link
Contributor Author

valgur commented Nov 4, 2024

Uhm... how should I handle this?

Also, I don't have any plans to open new PRs in addition to the existing ones any time soon (although I do have another couple hundred potential ones left in the tank - mostly minor recipe bug fixes related to cross-compilation support).

A job has not been automatically scheduled because author has too many pull requests open.
The team will launch this job manually once we are ready to review this PR - and no action is required.
Alternatively, you may choose to reduce your number of open PRs to the ones consider require more immediate attention.
Thank you for your contribution to Conan Center!

@AbrilRBS
Copy link
Member

AbrilRBS commented Nov 4, 2024

@valgur thanks for fixing the issues!

Right now no PRs are running by default, and we are triggering them by hand at first to get a handle of how the new CI behaves, regardless of how many prs the author has opened :)

I'll launch this one now, thanks!

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.

[request] libosmium/2.15.6
4 participants