Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts how the PYTHONOCC_WRAP_VISU CMake option is initialized so it properly defaults based on OpenGL detection while respecting user-provided values, and avoids assigning an invalid non-boolean default; also slightly restructures OpenGL discovery and messaging. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
PYTHONOCC_WRAP_DATAEXCHANGEoption is now declared twice in a row, which is redundant and could be confusing; please keep only a singleoption_with_default(PYTHONOCC_WRAP_DATAEXCHANGE ...)line. - When
PYTHONOCC_WRAP_VISUis explicitly set toONby the user,find_package(OpenGL)andinclude_directories(${OPENGL_INCLUDE_DIR})are skipped, which may leave the OpenGL headers and libraries unset even though visualization is enabled; consider still running the OpenGL discovery (and emitting a warning if it is not found) in that case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `PYTHONOCC_WRAP_DATAEXCHANGE` option is now declared twice in a row, which is redundant and could be confusing; please keep only a single `option_with_default(PYTHONOCC_WRAP_DATAEXCHANGE ...)` line.
- When `PYTHONOCC_WRAP_VISU` is explicitly set to `ON` by the user, `find_package(OpenGL)` and `include_directories(${OPENGL_INCLUDE_DIR})` are skipped, which may leave the OpenGL headers and libraries unset even though visualization is enabled; consider still running the OpenGL discovery (and emitting a warning if it is not found) in that case.
## Individual Comments
### Comment 1
<location path="CMakeLists.txt" line_range="87" />
<code_context>
+ find_package(OpenGL)
+ include_directories(${OPENGL_INCLUDE_DIR})
+ if(NOT OPENGL_FOUND)
+ message(WARNING "OpenGL library not found, Visualization compilation is turned OFF")
+ endif(NOT OPENGL_FOUND)
+endif(NOT DEFINED PYTHONOCC_WRAP_VISU)
#################
# Build options #
#################
include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/OCCT_Modules.cmake)
# add an option to choose toolkits to compile
-if(OPENGL_FOUND)
- option_with_default(PYTHONOCC_WRAP_VISU "Compile Visualisation" ON)
-else(OPENGL_FOUND)
- message(WARNING "OpenGL library not found, Visualization compilation is turned OFF")
- set(PYTHONOCC_WRAP_VISU "Compile Visualisation" OFF)
-endif(OPENGL_FOUND)
+option_with_default(PYTHONOCC_WRAP_VISU "Compile Visualisation" ${OPENGL_FOUND})
+option_with_default(PYTHONOCC_WRAP_DATAEXCHANGE "Compile DataExchange wrapper" ON)
option_with_default(PYTHONOCC_WRAP_DATAEXCHANGE "Compile DataExchange wrapper" ON)
</code_context>
<issue_to_address>
**nitpick (typo):** Inconsistent spelling of 'Visualization/Visualisation' in user-facing strings.
The warning uses "Visualization" while the option text uses "Visualisation". Please pick one (US or UK) and use it consistently across these user-facing strings.
```suggestion
option_with_default(PYTHONOCC_WRAP_VISU "Compile Visualization" ${OPENGL_FOUND})
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PYTHONOCC_WRAP_VISU is a regular option, ON by default if OpenGL is found: a value passed on the command line or in the cache is kept. It was overwritten by a normal variable when OpenGL was not found; asking for it without OpenGL is now a configuration error instead of a silent OFF. OpenGL is looked for once. See PR #1484.
|
Thanks. The non-empty string bug was fixed in the review/occt800 branch (OCCT 8 port), which also removed option_with_default, so this PR no longer applies. I made PYTHONOCC_WRAP_VISU a regular option defaulting to OPENGL_FOUND, so a value passed on the command line is kept, and asking for it without OpenGL is now an explicit configuration error instead of a silent OFF. |
Previously, the
PYTHONOCC_WRAP_VISUbuild setting was incorrectly set to a non-empty string instead of the valueOFFhere. This caused all subsequentif(PYTHONOCC_WRAP_VISU)checks to evaluate the variable as set.Furthermore, a user couldn't set
PYTHONOCC_WRAP_VISUon the command line, as it was always overwritten withinCMakeLists.txt.Summary by Sourcery
Fix configuration of the visualization wrapper build option to respect user overrides and correctly depend on OpenGL availability.
Bug Fixes:
Build: