Skip to content

scripts: add a way to provide libclang include dirs externally#63

Open
rmelotte wants to merge 1 commit intoArtifexSoftware:masterfrom
rmelotte:add-a-way-to-provide-libclang-include-dirs-externally
Open

scripts: add a way to provide libclang include dirs externally#63
rmelotte wants to merge 1 commit intoArtifexSoftware:masterfrom
rmelotte:add-a-way-to-provide-libclang-include-dirs-externally

Conversation

@rmelotte
Copy link
Contributor

@rmelotte rmelotte commented Mar 5, 2025

When cross-compiling, libclang doesn't necessarily have the right include directories configured by default.
See for example: https://bugs.ghostscript.com/show_bug.cgi?id=708041

Since we also have no way to figure out the right include directories from our scripts, add a new environment variable that can be used to provide them externally.

Copy link
Contributor

@julian-smith-artifex-com julian-smith-artifex-com left a comment

Choose a reason for hiding this comment

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

Many thanks for this PR.

Some minor suggestions:

  1. Could you call the environment variable MUPDF_LIBCLANG_ARGS? as it isn't inherently limited to include directories.
  2. Use shlex.split() instead of splitting simply by spaces, in case we ever need to pass args with spaces etc.
  3. We tend to use os.environ.get() instead of os.getenv().

So something like:

import shlex
...
libclang_args = os.environ.get('MUPDF_LIBCLANG_ARGS', '')
args += shlex.split(libclang_args)

Some non-technical things:

  1. To accept your PR we'll need you to agree to our Artifex Contributor License Agreement - see:
    https://artifex.com/contributor/

  2. This Github repostitory is really a readonly copy of the main repository ghostscript.com:/home/git/mupdf.git, so when your PR is ok i will push your commit to the main repository manually instead of merging the PR here.

  3. If you prefer, i can create a commit independently to save you from worrying about the CLA etc.

When cross-compiling, libclang doesn't necessarily have the right
include directories configured by default.
See for example: https://bugs.ghostscript.com/show_bug.cgi?id=708041

Since we also have no way to figure out the right include directories
from our scripts, add a new environment variable that can be used to
provide them externally.

Signed-off-by: Raphaël Mélotte <raphael.melotte@mind.be>
@rmelotte rmelotte force-pushed the add-a-way-to-provide-libclang-include-dirs-externally branch from 048855a to b2483d9 Compare March 6, 2025 08:45
@rmelotte
Copy link
Contributor Author

rmelotte commented Mar 6, 2025

Thanks for your excellent suggestions, I applied them and updated the PR.

Regarding the CLA, I actually already signed it using the CLA Assistant in the PyMuPDF project in pymupdf/PyMuPDF#3173.
Do I need to sign it again?

@rmelotte
Copy link
Contributor Author

rmelotte commented Apr 3, 2025

Hi @julian-smith-artifex-com ,
Did you get information regarding the CLA? Do I need to sign it again?

@julian-smith-artifex-com
Copy link
Contributor

Apologies for the delay. Yes, you've already signed the CLA so there's no need to do so again.

I'm off work for the next few days, but i've added a todo item to submit your commit for internal review, hopefully by the end of next week.

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