Skip to content

Add INTERFACE definition GLEW_NO_GLU for glew_head.h#426

Closed
WangWeiLin-MV wants to merge 1 commit intonigels-com:masterfrom
WangWeiLin-MV:cmake/public-definition/GLEW_NO_GLU
Closed

Add INTERFACE definition GLEW_NO_GLU for glew_head.h#426
WangWeiLin-MV wants to merge 1 commit intonigels-com:masterfrom
WangWeiLin-MV:cmake/public-definition/GLEW_NO_GLU

Conversation

@WangWeiLin-MV
Copy link
Copy Markdown

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

@nigels-com
Copy link
Copy Markdown
Owner

Looks good.

@dg0yt
Copy link
Copy Markdown

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.


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

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
Copy Markdown
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.

@WangWeiLin-MV WangWeiLin-MV closed this by deleting the head repository May 7, 2025
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