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

[Cmake] FindASS migrate to TARGET usage #23824

Merged
merged 3 commits into from Sep 27, 2023

Conversation

fuzzard
Copy link
Contributor

@fuzzard fuzzard commented Sep 26, 2023

Description

Migrates FindASS to full TARGET usage. Also add version requirement specification to ASS required_deps

Motivation and context

Migrate to more modern cmake. Accurately enforce minimum version of lib
@a1rwulf this should now correctly error out on incorrect version of libass before compilation failure.

How has this been tested?

Build macos aarch64
Build win x64

What is the effect on users?

N/A

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

Copy link
Member

@a1rwulf a1rwulf left a comment

Choose a reason for hiding this comment

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

Tested on Ubuntu Focal with libass 0.14.x - correctly bails with the version error.

CMake Error at /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
  Could NOT find ASS: Found unsuitable version "0.14.0", but required is at
  least "0.15.0" (found /usr/lib/x86_64-linux-gnu/libass.so)
Call Stack (most recent call first):
  /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:391 (_FPHSA_FAILURE_MESSAGE)
  cmake/modules/FindASS.cmake:94 (find_package_handle_standard_args)
  cmake/scripts/common/Macros.cmake:384 (find_package)
  cmake/scripts/common/Macros.cmake:400 (find_package_with_ver)
  CMakeLists.txt:232 (core_require_dep)

fuzzard added 3 commits September 27, 2023 07:23
…mpile projects

/usr was causing cmake to potentially search host system folders on linux jenkins projects
we dont want to add this unless its some non default path provided from configure.ac and the
dev
A hard requirement of libass 0.15.0 was brought in with PR xbmc#23421
Force version check on libass find
@fuzzard
Copy link
Contributor Author

fuzzard commented Sep 27, 2023

Jenkins build this please

@fuzzard
Copy link
Contributor Author

fuzzard commented Sep 27, 2023

Of note, 07d64d5 actually fixes an issue on the arm linux CI builder, that i cant for the life of me explain why it even works currently.

15:29:50 -- Found HarfBuzz: /home/jenkins/workspace/LINUX-ARM-GLES/tools/depends/xbmc-depends/arm-linux-gnueabihf-debug/lib/libharfbuzz.a (found version "3.1.1") 
15:29:50 -- Found Iconv: /usr/lib32/libc.so  
15:29:50 -- Found KissFFT: /home/jenkins/workspace/LINUX-ARM-GLES/xbmc/contrib

Which can be seen in last nightly for master https://jenkins.kodi.tv/job/LINUX/62367/consoleFull

@wsnipex
Copy link
Member

wsnipex commented Sep 27, 2023

I guess the correct iconv from the toolchain libc is linked and the obviously wrong from build host is just ignored

Copy link
Member

@wsnipex wsnipex left a comment

Choose a reason for hiding this comment

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

Lgtm. Out of interest: why the nocache on the find lib/path?

@fuzzard
Copy link
Contributor Author

fuzzard commented Sep 27, 2023

Yeah, so thats a bit questionable. There was a couple things going through my head leading into its usage. First is i would like to remove (as much as possible anyway) the use of find_library/find_path when either a pkgconfig or a cmake config is available. Both of those methods are able to create targets, and also bring in a lot more information than just finding a lib/include (think dependencies, compile defines). So the no_cache was an attempt to slowly make sure i dont have some hidden reliance on certain cache variables in the maze of macro/functions/scripts we have.

The second was an attempt to force target usage fully, which leans into the second part of the first reasoning to make sure some stuff isnt being papered over and im missing something as i change things.

I havent seen a marked increase in single generator config times, however i do believe its increased multi config generation time. I cant prove that this is due to that particular cause right now (findffmpeg is the largest time sink i can see right now, which as you know is super complex/fragile still), but its pretty likely that id roll back the no_cache usage when i get further with more of the above target usage and remove some of our legacy macro/functions. More just a safety net i guess.

@wsnipex
Copy link
Member

wsnipex commented Sep 27, 2023

Understood, all good

@fuzzard fuzzard merged commit da55810 into xbmc:master Sep 27, 2023
2 checks passed
@fuzzard fuzzard deleted the cmake_findlibass_update branch September 27, 2023 09:07
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.

None yet

3 participants