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

Fix Incorrect Include Directory Structure in harfbuzz Conan Recipe #20158 #20159

Closed

Conversation

tuduongquyet
Copy link
Contributor

@tuduongquyet tuduongquyet commented Sep 26, 2023

Specify library name and version: harfbuzz/8.1.1

Resolving #20158


@CLAassistant
Copy link

CLAassistant commented Sep 26, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Sep 26, 2023

I detected other pull requests that are modifying harfbuzz/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Sorry, the system is under maintenance and it doesn't accept builds right now.
Please, check https://status.conan.io to obtain more information.
Thanks for your understanding and help with the Conan Center Index!


Conan v2 pipeline ❌

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

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

See details:

Sorry, the system is under maintenance and it doesn't accept builds right now.
Please, check https://status.conan.io to obtain more information.
Thanks for your understanding and help with the Conan Center Index!

Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

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

on my machine, (ubuntu), I have:

$ cat /usr/lib/x86_64-linux-gnu/pkgconfig/harfbuzz.pc
prefix=/usr
exec_prefix=/usr
libdir=/usr/lib/x86_64-linux-gnu
includedir=/usr/include

Name: harfbuzz
Description: HarfBuzz text shaping library
Version: 2.7.4

Libs: -L${libdir} -lharfbuzz
Libs.private: -lm
Requires.private: glib-2.0 >= 2.19.1 freetype2 >= 12.0.6 graphite2 >= 1.2.0
Cflags: -I${includedir}/harfbuzz

so the include path which is passed is /usr/include/harfbuzz, which contains all the hb*.h files

Also, /usr/lib/x86_64-linux-gnu/cmake/harfbuzz/harfbuzz-config.cmake has:

set_target_properties(harfbuzz::harfbuzz PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_harfbuzz_prefix}/${_harfbuzz_includedir}/harfbuzz"
  ...)

Also also, https://github.com/harfbuzz/harfbuzz/blob/main/test/fuzzing/hb-draw-fuzzer.cc#L4 does not specify harfbuzz/ when including.

also also also, https://harfbuzz.github.io/a-simple-shaping-example.html does not specify harfbuzz/ when including either

Where does your assumption that it should be included as #include "harfbuzz/hb*.h come from ?

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 26, 2023

I agree with @ericLemanissier

Your PR is incorrect, and here is another proof from upstream CMakeLists:
https://github.com/harfbuzz/harfbuzz/blob/c8c97864e8c810068123ef62947be13675df54c2/CMakeLists.txt#L452-L454

These lines clearly say that CMake config files should expose headers under harfbuzz folder directly. You are breaking recipe actually here, while your incorrect include convention was also supported since recipe was kind enough to provide both "include" AND "include/harfbuzz" in cpp_info.includedirs. I don't know how you consume this recipe.

@tuduongquyet
Copy link
Contributor Author

I'm currently working on a project that involves using harfbuzz, and in the codebase, we have references to harfbuzz/hb.h. I also conducted a search on GitHub and found numerous instances where harfbuzz/hb.h is used in other projects as well. This seems to be a common convention when working with harfbuzz.

Furthermore, it's a widely followed practice to structure include paths with the format /

when dealing with packages. This helps maintain clarity and organization in larger codebases.

However, I should have also considered the specific structure of the harfbuzz Debian package, which utilizes Cflags: -I${includedir}/harfbuzz. In this case, the package itself defines the include path as /usr/include/harfbuzz, and it's acceptable to directly include headers without the harfbuzz/ prefix due to this packaging configuration.

So, while #include "harfbuzz/hb.h" is a common convention, the Debian package configuration you mentioned allows for a simplified #include "hb.h" when working within the context of an Ubuntu system with the harfbuzz package installed.

So I think we should keep the same.
I will close the issue and the PR.

Thank you for reviewing. @ericLemanissier

@tuduongquyet
Copy link
Contributor Author

Thank you @SpaceIm

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.

5 participants