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 INTERFACE definition GLEW_NO_GLU for glew_head.h #426

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

Conversation

WangWeiLin-MV
Copy link

@WangWeiLin-MV WangWeiLin-MV commented Feb 6, 2025

@nigels-com
Copy link
Owner

Looks good.

@dg0yt
Copy link

dg0yt commented Feb 8, 2025

This ignores the generated pkg-config.
(It also doesn't reach users of CMake's FindGLEW.)
And documentation still claims that glew.h include glu.h.

glew/doc/install.html

Lines 158 to 159 in 3da315c

In addition, <tt>glew.h</tt> includes <tt>glu.h</tt>, so you do not
need to include it separately.

@@ -159,6 +159,7 @@ target_link_libraries (glew_s ${GLEW_LIBRARIES})

target_compile_definitions(glew_s INTERFACE "GLEW_STATIC")
foreach(t glew glew_s)
target_compile_definitions(${t} INTERFACE GLEW_NO_GLU)
Copy link

Choose a reason for hiding this comment

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

Suggested change
target_compile_definitions(${t} INTERFACE GLEW_NO_GLU)
target_compile_definitions(${t} PUBLIC GLEW_NO_GLU)

and remove the directory-scoped definition instead.

@nigels-com
Copy link
Owner

nigels-com commented Feb 8, 2025

This ignores the generated pkg-config.

Indeed. There is a nuance with this, from my point of view. The official release of GLEW does indeed continue the (arguably obsolete) glu.h dependency for compatibility reasons. That's the compatibility story. Old client code depending on GLU ought to continue building and linking.

The cmake is opt-in and (as I recall the previous discussion) more free to behave in a contemporary, convenient, modernised manner. For modern or new code using cmake and GLEW I do think GLEW_NO_GLU is a reasonable assumption, and most likely what most people want in practice.

We could discuss the merits of that nuance in the context of the realities of 2025. But I did want to point out this somewhat convoluted way for compatibility and modernity to coexist reasonably and pragmatically.

It's also one reason the two builds continue along side each other, to serve those different needs.

Edit: And thanks for the review, always happy to have additional eyes on changes.

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.

3 participants