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: Allow ignoring system packages #11955

Merged
merged 2 commits into from Jun 30, 2023

Conversation

TellowKrinkle
Copy link
Contributor

@TellowKrinkle TellowKrinkle commented Jun 15, 2023

Sometimes, you want to ignore system packages

  • If something is breaking on your builds, but not on the CI's, disabling all system packages is a fast way to test if that's the cause
  • Sometimes, libraries make API-incompatible changes, and Linux distros may update before we have a chance to ensure compatibility. User reactions to this range from screaming at us to trying to force-downgrade the package globally, and it would be very nice to be able to offer "add -DUSE_SYSTEM_LIBMGBA=OFF to your cmake args" as a cleaner and simpler solution.
  • When building for distribution macOS, you often don't want to pick up libraries from homebrew / other 3rd party package managers

This adds two sets of cmake args:

  • -DUSE_SYSTEM_LIBS=(ON|OFF|AUTO): Global control for all system libs at once
  • -DUSE_SYSTEM_<library>=(ON|OFF|AUTO) Control for individual libraries (overrides global if set)

AUTO acts like the current default (and remains the default), using the system library if found
ON acts like our current APPROVED_VENDORED_DEPENDENCIES mode, failing configure if the system package is not found
OFF disables searching for the system package completely, picking our bundled version instead

Other Changes

  • Since this offers the same functionality as APPROVED_VENDORED_DEPENDENCIES, I've deprecated that option, though it does still work for now (with a deprecation message).
  • Since you can now disable system packages, I've removed the blocks for a number of system packages on macOS. Do not merge until I've verified that none of these are installed on our CI (or if they are installed, that we add the appropriate -DUSE_SYSTEM_XXX=OFF in the CI scripts). (This will remain a draft until then) It looks like the CI is all good, there's a homebrew libLZMA that we should be disabling, but it's also getting picked up by the current build system, so this shouldn't be making things worse.
  • In order to more easily wrap the new (more complicated) handling into functions, I've also migrated all optionally-bundled external libs to CMake imported targets, and made them use either a find script or pkg-config import, rather than manual search. This allows all packages to be properly searched for and imported by one of two functions (one for find scripts, the other for pkg-config), with no special handling for either of the two sides to e.g. add include directores.
    • For libraries that have added targets in newer versions of their system cmake find scripts, I've tried to match names to those versions, so if we ever raise the minimum version past the one that added targets, we should be able to delete our find scripts without changing anything else.

@TellowKrinkle TellowKrinkle marked this pull request as draft June 15, 2023 06:28
@TellowKrinkle TellowKrinkle force-pushed the CMakeDependencies branch 2 times, most recently from 6a39996 to 1deabc2 Compare June 15, 2023 06:40
@AdmiralCurtiss
Copy link
Contributor

I have not checked any of the code but this gets a conceptual approval from me, what you've described seems much better than what we've been doing before.

@TellowKrinkle TellowKrinkle marked this pull request as ready for review June 17, 2023 01:56
@jmallach
Copy link
Contributor

@TellowKrinkle our MRs were quite parallel in time. Can you make sure this takes into account #11950? Thanks!

@AdmiralCurtiss AdmiralCurtiss merged commit fa81006 into dolphin-emu:master Jun 30, 2023
14 checks passed
@AdmiralCurtiss
Copy link
Contributor

@TellowKrinkle As far as I can tell you have not touched gtest here so that still uses the old system, feel free to send a follow-up to convert it to the new system.

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