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 harfbuzz library #115

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Add harfbuzz library #115

merged 6 commits into from
Apr 18, 2024

Conversation

tpimh
Copy link
Contributor

@tpimh tpimh commented Feb 25, 2024

Again, I haven't yet tested if it works, so I am creating this PR as a draft. Just want to know if anything should be changed.

@tpimh
Copy link
Contributor Author

tpimh commented Feb 27, 2024

Testing it now. It seems that harfbuzzConfig.cmake references Threads::Threads, and pspsdk doesn't provide FindThreads.cmake. There probably should be a workaround for it.

Anyway, my test code is here (based on a project by @sharkwouter), so if anyone wants to try and test this library, it should be a good start.

@sharkwouter
Copy link
Member

So for threads we have pthreads, which is also used in C++ by threads. You can find the source here: https://github.com/pspdev/pthread-embedded/tree/platform_agnostic

@sharkwouter
Copy link
Member

sharkwouter commented Feb 27, 2024

So, I installed both sdl2-ttf and harfbuzz from the automated build and this is the result I got:

$ psp-cmake ..
-- The C compiler identification is GNU 13.2.0
-- The CXX compiler identification is GNU 13.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/wouter/Personal/pspdev/bin/psp-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/wouter/Personal/pspdev/bin/psp-g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found PkgConfig: /home/wouter/Personal/pspdev/bin/psp-pkg-config (found version "0.29.2") 
-- Checking for one of the modules 'sdl2'
-- Found Freetype: /home/wouter/Personal/pspdev/psp/lib/libfreetype.a (found version "2.11.0") 
CMake Error at CMakeLists.txt:11 (find_package):
  By not providing "FindHarfbuzz.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Harfbuzz",
  but CMake did not find one.

  Could not find a package configuration file provided by "Harfbuzz" with any
  of the following names:

    HarfbuzzConfig.cmake
    harfbuzz-config.cmake

  Add the installation prefix of "Harfbuzz" to CMAKE_PREFIX_PATH or set
  "Harfbuzz_DIR" to a directory containing one of the above files.  If
  "Harfbuzz" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
See also "/home/wouter/Personal/jewels-SDL2-PSP-test/psp/CMakeFiles/CMakeOutput.log".

@tpimh
Copy link
Contributor Author

tpimh commented Feb 27, 2024

My bad, for some reason on Ubuntu CMake is okay with "Harfbuzz" package, but for PSP it requires lowercase "harfbuzz". Not sure why CMake behaves differently, but I have updated CMakeLists.txt now.

@tpimh
Copy link
Contributor Author

tpimh commented Feb 28, 2024

Two more things that are unwanted are these warnings:

==> Checking for packaging issues...
==> WARNING: Package contains reference to $srcdir
psp/lib/libharfbuzz.a
psp/lib/libharfbuzz-subset.a
==> WARNING: Package contains reference to $pkgdir
psp/lib/pkgconfig/harfbuzz-subset.pc
psp/lib/pkgconfig/harfbuzz.pc

Possibly something is not right during prepare() step?

@tpimh
Copy link
Contributor Author

tpimh commented Feb 28, 2024

I have actually compiled my example and run it in an emulator. Here's the patch that I needed:

--- /usr/local/pspdev/psp/lib/cmake/harfbuzz/harfbuzzConfig.cmake
+++ harfbuzzConfig.cmake
@@ -59,8 +59,8 @@
 add_library(harfbuzz::harfbuzz STATIC IMPORTED)
 
 set_target_properties(harfbuzz::harfbuzz PROPERTIES
-  INTERFACE_INCLUDE_DIRECTORIES "/harfbuzz"
-  INTERFACE_LINK_LIBRARIES "Threads::Threads;/usr/local/pspdev/psp/lib/libfreetype.a"
+  INTERFACE_INCLUDE_DIRECTORIES "/usr/local/pspdev/psp/include/harfbuzz"
+  INTERFACE_LINK_LIBRARIES "/usr/local/pspdev/psp/lib/libfreetype.a;/usr/local/pspdev/psp/lib/libpng.a;/usr/local/pspdev/psp/lib/libz.a;/usr/local/pspdev/psp/lib/libbz2.a"
 )
 
 if(CMAKE_VERSION VERSION_LESS 2.8.12)

@sharkwouter
Copy link
Member

Not sure if you're still working on this, but you can add the patch to this repo, no problem. For an example you could look at SDL2.

@tpimh
Copy link
Contributor Author

tpimh commented Mar 3, 2024

@sharkwouter I will add the patch, I think it is still missing threads. It's just the example that is not using them, but anything that relies on threads would fail. It shouldn't be hard to add.

I can probably finalize this later today, and then try to link SDL2_ttf against harfbuzz.

@tpimh
Copy link
Contributor Author

tpimh commented Mar 3, 2024

  • fix Package contains reference to $pkgdir
  • fix Package contains reference to $srcdir
  • fix INTERFACE_INCLUDE_DIRECTORIES and INTERFACE_LINK_LIBRARIES in generated cmake file

@sharkwouter
Copy link
Member

I think the cmake file containing direct references to full paths is normal, since we have that for more packages but it does not seem to cause problems.

I've added a patch which fixes the issue with it trying to link to threads. Hope that helps. I would not try to fix references to the srcdir in .a files.

@sharkwouter
Copy link
Member

sharkwouter commented Mar 18, 2024

Can you add hardbuzz as a dependency for freetype? That is still missing.

@tpimh
Copy link
Contributor Author

tpimh commented Mar 19, 2024

Yes, if this fix is okay with you, then it's fine with me as well. I think harfbuzz should also be added as a dependency for SDL2_ttf, I'll see if it builds and works.

@tpimh tpimh marked this pull request as ready for review March 19, 2024 12:34
Copy link
Member

@sharkwouter sharkwouter left a comment

Choose a reason for hiding this comment

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

Could you still add harfbuzz as dependency for sdl2_ttf?

@tpimh
Copy link
Contributor Author

tpimh commented Mar 21, 2024

Sure, I can do it as a part of this pull request as well.

@tpimh
Copy link
Contributor Author

tpimh commented Mar 26, 2024

Let's see if this test still fails when SDL2-ttf is built with harfbuzz: #45 (comment)

Needs to be tested on real hardware, but at least it works on emulator:
Screenshot from 2024-03-26 15-06-48

@tpimh
Copy link
Contributor Author

tpimh commented Mar 27, 2024

@sharkwouter I would probably still add this patch to package step: #115 (comment). I know it's ugly, but I have no idea how to implement it better (and neither does the main harfbuzz dev: harfbuzz/harfbuzz#4613 (comment)).

@sharkwouter
Copy link
Member

I added a fix for psp-pkg-conf outputting something that does not work. I'm testing now.

@sharkwouter
Copy link
Member

@sharkwouter I would probably still add this patch to package step: #115 (comment). I know it's ugly, but I have no idea how to implement it better (and neither does the main harfbuzz dev: harfbuzz/harfbuzz#4613 (comment)).

@tpimh the fix-cmake patch file already addresses this issue.

Copy link
Member

@sharkwouter sharkwouter left a comment

Choose a reason for hiding this comment

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

We have a dependency loop. Harfbuzz relies on Freetype2 and Freetype2 relies on Harfbuzz. Our build script cannot handle that.

What can we do with this? Does it have to be like this?

@tpimh
Copy link
Contributor Author

tpimh commented Mar 27, 2024

The fix-cmake patch only removes "Threads::Threads" from INTERFACE_LINK_LIBRARIES, but doesn't add libpng, libz and libbz2; and it doesn't change INTERFACE_INCLUDE_DIRECTORIES. Not that big of a deal, but a minor annoyance.

I can remove harfbuzz dependency from freetype2, and sdl2-ttf should still build fine. Need to check how other distros handle such circular dependencies.

@tpimh tpimh force-pushed the harfbuzz branch 2 times, most recently from f1981a3 to 69a0db0 Compare April 15, 2024 10:45
@tpimh
Copy link
Contributor Author

tpimh commented Apr 15, 2024

Even after removing the circular dependency, the build system was not able to build the package without extra complications.

  1. Duplicate dependencies are not supported. sdl2-ttf depends on sdl2, freetype2 and harfbuzz, so the dependency list generated looks like libpng libpng freetype2 libpng harfbuzz freetype2 sdl2 sdl2-ttf. The first libpng builds fine, the second fails with "A package has already been built".
  2. Preventing the duplicates to be added to the dependency list, only complicates things, because now the dependencies are not in the correct order: libpng harfbuzz freetype2 sdl2 sdl2-ttf. harfbuzz depends on freetype2, but is being built before it. Building it fails with "Could not resolve all dependencies".
  3. Removing freetype2 dependency from sdl2-ttf fixes the build. The dependency list is now libpng freetype2 harfbuzz sdl2 sdl2-ttf, the order is correct, because freetype2 is added as dependency of harfbuzz.

@tpimh tpimh requested a review from sharkwouter April 16, 2024 07:30
@sharkwouter sharkwouter merged commit 31d4a3c into pspdev:master Apr 18, 2024
63 checks passed
@sharkwouter
Copy link
Member

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.

2 participants