fix: correctly set default PYTHONOCC_WRAP_VISU build option#1484
Open
justend29 wants to merge 1 commit intotpaviot:masterfrom
Open
fix: correctly set default PYTHONOCC_WRAP_VISU build option#1484justend29 wants to merge 1 commit intotpaviot:masterfrom
justend29 wants to merge 1 commit intotpaviot:masterfrom
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.
| 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}) |
There was a problem hiding this comment.
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.
Suggested change
| option_with_default(PYTHONOCC_WRAP_VISU "Compile Visualisation" ${OPENGL_FOUND}) | |
| option_with_default(PYTHONOCC_WRAP_VISU "Compile Visualization" ${OPENGL_FOUND}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: