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

Check for existence of libraries before parsing their flags with pkg-config #56532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madmiraal
Copy link
Contributor

As requested here:

# FIXME: Check for existence of the libs before parsing their flags with pkg-config

This PR basically replaces:

if not env["builtin_<library>"]:
    `env.ParseConfig("pkg-config <library> --cflags --libs")`

with

if not env["builtin_<library>"]:
    error = os.system("pkg-config <library> --exists")
    if error:
        print("Error: <library>-dev library not found. Using builtin <library>.")
        env["builtin_<library>"] = True
    else:
        env.ParseConfig("pkg-config <library> --cflags --libs")

When there are dependencies it checks for all the libraries and only if they are all requested and all present does it use the shared libraries. It also provides useful information about which libraries are missing and which were not requested.

When there is a minimum version required the check is changed to:

error = os.system("pkg-config <library> --atleast-version='<version>'")

if False: # not env['builtin_assimp']:
# FIXME: Add min version check

I've removed the check for assimp instead of fixing it, because it was removed in #44599 (#42941 in 3.x).

I've made all the checks consistent (including the initial ones in can_build()).

The main difference this PR makes is that when pkg-config <library> --exists returns an error, it reports the error and then falls back to using the builtin library instead of failing and not building at all. See #56526 and #7373.

@Riteo
Copy link
Contributor

Riteo commented Jul 3, 2023

Oof this one limbo'ed hard.

Yeah the TODO's still there and the logic looks very similar. It will still probably require a good rebase though.

Looks a bit verbose to me but considering the rest of the script and the specific requirements that some dependencies have I can't really see a worthwhile solution.

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.

3 participants