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 librealsense/2.40.0 #3868

Closed
wants to merge 9 commits into from
Closed

Add librealsense/2.40.0 #3868

wants to merge 9 commits into from

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Dec 12, 2020

librealsense/2.40.0

  • 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
Copy link
Collaborator

Some configurations of 'librealsense/2.40.0' failed in build 1 (aefa80053d49270feda4008f479d62412091b9c1):

cmake = self._configure_cmake()
cmake.install()
tools.rmdir(os.path.join(self.package_folder, "lib", "pkgconfig"))
tools.rmdir(os.path.join(self.package_folder, "lib", "cmake"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explicitly set the cmake_find_package and pkg_config generator name in package_info?
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

#include "l500-serializable.h"
-#include "../../../third-party/json.hpp"
+#include "../../third-party/json.hpp"

Copy link
Contributor

Choose a reason for hiding this comment

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

I have 2 questions about this:

  • is this a bug in their code as the relative path is wrong
  • what 3rd party json lib is used? Does conan have it already?

Copy link
Contributor

@prince-chrismc prince-chrismc Dec 12, 2020

Choose a reason for hiding this comment

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

I think it's the excessive use of PROJECT_SOURCE_DIR +> to realative path + the CCI cmake wrapper https://github.com/IntelRealSense/librealsense/search?q=PROJECT_SOURCE_DIR

Yes but we dont have anything old
https://github.com/IntelRealSense/librealsense/blob/34fc284d537a4b873a37b737a61f1f1da92dbb60/third-party/json.hpp#L4

Copy link
Contributor

Choose a reason for hiding this comment

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

We have 3.4-3.9. I guess that's too recent and breaking the api?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep!

There are a lot of 3rd party that are embedded, I am not sure it thats a blocker 🚫

- include(libusb_config)
- target_link_libraries(${LRS_TARGET} PRIVATE usb)
+ find_package(libusb REQUIRED)
+ target_link_libraries(${LRS_TARGET} PRIVATE libsusb)
Copy link
Contributor

@madebr madebr Dec 12, 2020

Choose a reason for hiding this comment

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

If this is using a Conan cmake_find_package, then you're linking to a library instead of a library target


set_target_properties (${PROJECT_NAME} PROPERTIES FOLDER Library)

+target_link_libraries(${PROJECT_NAME} Boost::Boost lz4::lz4)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, conan's boost had components now so this is header only

+ target_link_libraries(${LRS_TARGET} PRIVATE libsusb)
if(USE_EXTERNAL_USB)
- add_dependencies(${LRS_TARGET} libusb)
+ add_dependencies(${LRS_TARGET} libusb)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since libusb is the lib.a there's no cmake target and there's no dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this does nothing. There is no other target to consider/build in the dependency tree.

Comment on lines +48 to +49
if self.settings.os != "Windows":
self.requires("libusb/1.0.23")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is libusb not needed on Windows?

Btw, the libusb library on Linux is libusb.a (so linkable in cmake by adding usb to target_link_libraries).
On MSV it is called libusb.lib, so there it should be target_link_libraries(... libusb).

@stale
Copy link

stale bot commented Jan 26, 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 Jan 26, 2021
@stale
Copy link

stale bot commented Feb 25, 2021

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

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

Successfully merging this pull request may close these issues.

4 participants