Skip to content

Conversation

@bellenot
Copy link
Member

@bellenot bellenot commented Nov 5, 2025

  • Move TGLWSIncludes.h from the inc to the src directory
  • Don't install the GLEW headers

- Move `TGLWSIncludes.h` from the `inc` to the `src` directory
- Don't install the GLEW headers
@bellenot bellenot requested review from hahnjo and silverweed November 5, 2025 12:46
@bellenot bellenot self-assigned this Nov 5, 2025
@bellenot bellenot requested a review from couet as a code owner November 5, 2025 12:46
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

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

Thanks

@github-actions
Copy link

github-actions bot commented Nov 5, 2025

Test Results

    20 files      20 suites   3d 14h 7m 30s ⏱️
 3 707 tests  3 707 ✅ 0 💤 0 ❌
72 342 runs  72 342 ✅ 0 💤 0 ❌

Results for commit e16b358.

@dpiparo dpiparo merged commit f7eda99 into root-project:master Nov 5, 2025
25 of 27 checks passed
@bellenot bellenot deleted the fix-glew-deprecation branch November 5, 2025 15:31
@smuzaffar
Copy link
Contributor

smuzaffar commented Nov 7, 2025

Hi @bellenot , while testing latest root mater changes , we get build errors in CMSSW [a]. The root change set which we are testing in cmssw is 4ee5e77...59b8b01 . I think this error might be a side effect of change here. Any idea why this header is now needed ?

[a]

In file included from src/Fireworks/Core/src/CmsAnnotation.cc:4:
.../lcg/root/6.39.1-f6b681b05d584b7e93413a4cff8a643d/include/TGLIncludes.h:21:10: fatal error: GL/glew.h: No such file or directory
   21 | #include <GL/glew.h>

@smuzaffar
Copy link
Contributor

TGLIncludes.h header in root has not changed in years , so may be something wrong on our side. Let me first check if this still works with previous commit of root

@smuzaffar
Copy link
Contributor

ah previously GL/glew.h header was part of root distribution but it is not any more. So are we suppose to get glew-devel from system ?

@guitargeek
Copy link
Contributor

Yes, I guess you don't build ROOT with builtin_glew on?

@smuzaffar
Copy link
Contributor

we do build ROOT with builtin_glew https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_16_0_X/rootmaster/root.spec#L91

  -Dbuiltin_zlib=OFF \
  -Dbuiltin_lzma=OFF \
  -Dbuiltin_gsl=OFF \
  -Dbuiltin_glew=ON \
  -Dbuiltin_ftgl=ON \
  -Dbuiltin_gl2ps=ON \
  -Dbuiltin_xxhash=ON \

@smuzaffar
Copy link
Contributor

cmake also confirms this
-- Enabled support for: asimage asimage_tiff builtin_clang builtin_cling builtin_civetweb builtin_ftgl builtin_gl2ps builtin_glew builtin_llvm builtin_openui5 builtin_xxhash check_connection clad dataframe davix dcache fftw3 gdml geom http imt mathmore opengl pyroot roofit root7 runtime_cxxmodules shared spectrum ssl thisroot_scripts tmva tmva-cpu tmva-cudnn tmva-pymva tpython use_gsl_cblas webgui x11 xml xrootd

@bellenot
Copy link
Member Author

bellenot commented Nov 7, 2025

@smuzaffar Weird, it looks like GLEW_INCLUDE_DIR (or GLEW_INCLUDE_DIRS) is missing, somehow. I'll check

@guitargeek
Copy link
Contributor

Wait, isn't it just because this PR removed installing of the glew headers in builtins/glew/CMakeLists.txt?

So no matter what the include path is, the headers won't be found because ROOT doesn't ship them anymore.

@guitargeek
Copy link
Contributor

This PR did two things in one commit that seem unrelated to me:

  • Move TGLWSIncludes.h from the inc to the src directory

    • Don't install the GLEW headers

The TGLWSIncludes.h header was properly deprecated with an entry in the 6.38 release notes:
https://github.com/root-project/root/blob/master/README/ReleaseNotes/v638/index.md?plain=1#L44

Installing the builtin glew headers was not deprecated yet, so this change comes at a surprise. But I agree it's a good change, because we want to replace glew with glad anyway! But I think we are missing a few things in the release notes:

  • deprecate the installed glew headers in 6.38 with announcement in the release notes
  • mention in the 6.40 release notes that the builtin glew headers were removed (as done in this PR)

As a consequence, we also need to deprecate the TGLIncludes.h header, because this was a wrapper for the builtin <GL/glew.h>. I can take care of that stuff, because I started the whole chain with the deprecation of TGLWSIncludes.h 🙂

On the CMSSW side, glew is not used anyway, so I'll remove some includes there to make it compile. I'll open a PR to CMSSW.

@bellenot
Copy link
Member Author

bellenot commented Nov 7, 2025

@smuzaffar @guitargeek See #20323

guitargeek added a commit to guitargeek/cmssw that referenced this pull request Nov 7, 2025
The Frame Buffer Object (FBO) feature is part of OpenGL since version
3.0 from 2008. Nowadays, we can therefore assume that this feature
is always available. That means we don't have to use GLEW to check
whether this feature is present.

Like that, we can avoid using GLEW entirely, which is an implementation
detail of OpenGL loading in ROOT that will soon go away.

To make sure GLEW is not included anymore, this commit replaces includes
of `TGLIncludes.h` with the OpenGL headers only, because `TGLIncludes.h`
was including OpenGL plus GLEW.

See also:

  * root-project/root#18471
  * root-project/root#20294 (comment)
@guitargeek
Copy link
Contributor

@smuzaffar, I think this PR should fix the situation for CMSSW:

That gives us more time to think calmly what to do on the ROOT side.

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.

5 participants