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

add OGRE_PREFIX_DIR/include to cmake include dirs #2411

Closed
wants to merge 1 commit into from

Conversation

v4hn
Copy link

@v4hn v4hn commented Mar 21, 2022

make exported include paths consistent between pkg-config and cmake.

OGRE.pc exports both paths and there is code around that includes OGRE headers via #include <OGRE/...>.

Either make it clear that OGRE/ is no valid include path (and thus invalid in pkg-config as well) or merge&release this fix please.
As a downstream user, I prefer the additional prefix for better modularity.

make exported include paths consistent between pkg-config and cmake.

OGRE.pc exports both paths and there is code around that includes OGRE headers via `#include <OGRE/...>`.
@paroj paroj marked this pull request as draft March 21, 2022 17:07
@paroj paroj added this to the 13.x milestone Mar 21, 2022
@paroj
Copy link
Member

paroj commented Mar 21, 2022

OGRE.pc is wrong. OGRE/ should not be part of the include path. It should follow whatever we do in CMake.

I scheduled changing this for 13.4, as this will probably break some downstream.

@v4hn
Copy link
Author

v4hn commented Mar 21, 2022

Thanks for the clarification. it is obviously your decision which include paths you specify as the interface. Are there downsides to the prefix, aside from the name redundancy?

An annoying consequence of this decision is that you cannot really get rid of or point out projects that include headers with the wrong prefix as long as Linux distributions bundle the folder as /usr/include/OGRE. They will however break with custom installs of ogre. My proposal would have kept these builds working.

thanks lots for the quick response!

@paroj
Copy link
Member

paroj commented Mar 21, 2022

all inner Ogre #includes omit the OGRE/ prefix, so you must have OGRE/ as part of your includes for things to work out anyway.

If you dont really care about your project, you can always add ${OGRE_INCLUDE_DIRS}/../ to your includes, which will keep existing code working.

@v4hn
Copy link
Author

v4hn commented Mar 22, 2022

all inner Ogre #includes omit the OGRE/ prefix, so you must have OGRE/ as part of your includes for things to work out anyway.

that's true, even though it's only a problem for some headers. most use #include "Ogre...", which picks up headers relative to the current include location. some headers in subfolders actually use #include "../Ogre..." which works without the OGRE/ include path as well.
Most others in subfolders do not use this include style though.

@paroj
Copy link
Member

paroj commented Apr 2, 2022

fixed by dea94de

@paroj paroj closed this Apr 2, 2022
@v4hn
Copy link
Author

v4hn commented Apr 3, 2022

unless I misread that commit, you accidentally removed the wrong include path (by your explanation above) in the pkgconfig file, didn't you?

@paroj
Copy link
Member

paroj commented Apr 3, 2022

no, only the dark red part is being removed

@v4hn
Copy link
Author

v4hn commented Apr 3, 2022

You are right. Never read commits on smartphones right after waking up... Sorry for all the noise on this minor detail.

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