Skip to content

Stop leaking internal headers to other libraries. - #729

Merged
NWilson merged 7 commits into
PCRE2Project:masterfrom
feuerste:bazel_fix
Mar 17, 2025
Merged

Stop leaking internal headers to other libraries.#729
NWilson merged 7 commits into
PCRE2Project:masterfrom
feuerste:bazel_fix

Conversation

@feuerste

Copy link
Copy Markdown
Contributor

This is important for e.g. glib/gio, which also
has an internal config.h, and fails to compile, as it often picks up the pcre2 config.h first.

This is important for e.g. glib/gio, which also
has an internal config.h, and fails to compile, as
it often picks up the pcre2 config.h first.
@feuerste

Copy link
Copy Markdown
Contributor Author

@NWilson Can you please have a look? Thanks a lot!

@NWilson

NWilson commented Mar 14, 2025

Copy link
Copy Markdown
Member

Interesting. I see the issue. I don't know Bazel at all, but I can see that it's documented for the includes to be exported as part of the library's public interface.

From a little bit of Googling, I'm concerned about:

  • Is the -I syntax truly universal? For example, on Windows /I<path> is the standard commandline syntax. Does Bazel magically cover over this difference by parsing the COPTS and translating them to a toolchain-appropriate format? I can't find any docs on a list of supported COPTS flags. So, I worry there are places where this won't work.
  • Another alternative I've seen online is to make an internal dummy cc_library with the config header, which the real library would depend on. That's a bit like the pcre2test_dotc_headers hack I added - something similar could work to encapsulate the include directory in a private way.

@feuerste

Copy link
Copy Markdown
Contributor Author

Thanks for looking into this!

  • Is the -I syntax truly universal? For example, on Windows /I<path> is the standard commandline syntax. Does Bazel magically cover over this difference by parsing the COPTS and translating them to a toolchain-appropriate format? I can't find any docs on a list of supported COPTS flags. So, I worry there are places where this won't work.

I've seen it in quite a few bazel projects and MSVC according to https://learn.microsoft.com/en-us/cpp/build/reference/compiler-option allows both / and - to specify compiler options, but you are right, the other option below is a bit cleaner.

  • Another alternative I've seen online is to make an internal dummy cc_library with the config header, which the real library would depend on. That's a bit like the pcre2test_dotc_headers hack I added - something similar could work to encapsulate the include directory in a private way.

I changed it to use an impl library which is solely for internal use. Please take another look!

@NWilson

NWilson commented Mar 17, 2025

Copy link
Copy Markdown
Member

I have pushed a commit to your branch, which is my attempt to make the diff a bit smaller, by only pulling out the headers into their own internal dependency.

I am able to run the bazelisk build //... and bazelisk test //... commands locally.

I don't know enough about Bazel to confirm whether or not your original issue is fixed however (the public export of config.h to downstream clients).

@NWilson

NWilson commented Mar 17, 2025

Copy link
Copy Markdown
Member

Also @feuerste, if you're a Bazel user, do you know if it's OK now to remove the WORKSPACE.bazel file? I think this is some old compatibility thing for Bazel <6.3 (according to the docs), and those versions are now out of support I believe.

@feuerste

Copy link
Copy Markdown
Contributor Author

I have pushed a commit to your branch, which is my attempt to make the diff a bit smaller, by only pulling out the headers into their own internal dependency.

I am able to run the bazelisk build //... and bazelisk test //... commands locally.

I don't know enough about Bazel to confirm whether or not your original issue is fixed however (the public export of config.h to downstream clients).

I just tested it locally with all my dependencies, and it still works, so from my side we can merge it.

@feuerste

Copy link
Copy Markdown
Contributor Author

Also @feuerste, if you're a Bazel user, do you know if it's OK now to remove the WORKSPACE.bazel file? I think this is some old compatibility thing for Bazel <6.3 (according to the docs), and those versions are now out of support I believe.

I'm pretty sure we can remove that file, probably in a separate PR?
@mering, can you confirm?

@NWilson

NWilson commented Mar 17, 2025

Copy link
Copy Markdown
Member

I just tested it locally with all my dependencies, and it still works, so from my side we can merge it.

Thank you very much! I'll merge.

I'll open a separate PR for removing WORKSPACE.bazel.

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