- 
                Notifications
    You must be signed in to change notification settings 
- Fork 692
Modernize CMake scripts #439
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
Conversation
9bd19a4    to
    7cd022d      
    Compare
  
    | I believe this might actually solve all the issues you listed. :) Btw, I just force-pushed the branch because there was a typo in the commit message and I also forgot a closing parenthesis in the octovis config script (which only causes an issue if the system QGLViewer is used). I'll gladly fix any other bugs you encounter. I also made some design choices which I think are good but YMMV. I'm open to suggestions if you think something should be solved differently. | 
| @roehling, sounds great! I just merged some smaller cleanups and fixed CI calls into 'devel', could you rebase on these changes? | 
This is a rewrite of the CMake configuration that removes a lot of legacy code which is no longer needed and makes the build system more convoluted than necessary. By switching to modern CMake semantics, we gain the following advantages: * Include directories and dependent libraries are associated with the exported target, no additional configuration beyond target_link_libraries() required. * Built-in Qt MOC and UIC handling * Proper separation of build and install interface; this also makes RPATH just work out of the box without additional configuration. * Native build of vendored QGLViewer library, no need to call qmake and import the result. * Proper handling and versioning of dependencies. No hard-coded build paths showing up in unexpected places. Signed-off-by: Timo Röhling <[email protected]>
This is needed for Qt6 support. Signed-off-by: Timo Röhling <[email protected]>
7cd022d    to
    a32dcde      
    Compare
  
    | Done. I also updated QGLViewer to the latest release 2.9.1, because the currently vendored copy does not support Qt6. | 
| 
 Perfect, now all CI checks are green as well (and the windows one will probably as well once re-activated - since #321 might be fixed). Regarding the latest change added for Qt, I'm thinking about setting Qt5 as default (#442), depending on what's mostly used by the consumers e.g. in ROS distros and what creates fewer issues or hiccups. Using the system-provided QGLViewer should be preferred. Thoughts on this? | 
| Also friendly ping to @clalancette, just in case there is input from your side to consider regarding ROS integration. | 
| Taking the external QGLViewer library Qt version into consideration is a good point. I have an idea how to do that, I'll push an update later. | 
Signed-off-by: Timo Röhling <[email protected]>
| So I've added some code to detect the QQLViewer Qt version (very naively from the library filename) and turned the Qt version selector option into a tri-state ("auto", "5", "6"). By default, octovis now looks for the system QGLViewer library. If found, it forces a matching Qt version. If not found, it will build QGLViewer from source and pick Qt 5 over Qt 6. The pure CMake CI build jobs started failing because I am now treating all missing dependencies as errors. Before, the octovis build was silently skipped. | 
| @roehling, thanks, sounds useful! I think when building the whole distribution, the previous behavior of skipping octovis if the dependencies were missing would be preferable, since it's just an additional tool with rather large set of additional dependencies. I'll also try to add CI build for the single packages, just in case. Note that I also left a few code review comments above, not sure if you got a notification about them. | 
| 
 I can revert that change. 
 No, I did not, and I can't seem to find them at all, sorry. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roehling testing the review feature, please check comments in the review.
| 
 My bad, apparently the comments are drafts and only visible to me, until I click "Submit review". Are they now visible here or in the changed files? | 
Signed-off-by: Timo Röhling <[email protected]>
Signed-off-by: Timo Röhling <[email protected]>
Signed-off-by: Timo Röhling <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roehling Thanks for your contributions, you even greatly improved the version bump helper!
Ready to merge? From my side all fine!
This is a rewrite of the CMake configuration that removes a lot of legacy code which is no longer needed and makes the build system more convoluted than necessary.
By switching to modern CMake semantics, we gain the following advantages:
Include directories and dependent libraries are associated with the exported target, no additional configuration beyond target_link_libraries() required.
Built-in Qt MOC and UIC handling
Proper separation of build and install interface; this also makes RPATH just work out of the box without additional configuration.
Native build of vendored QGLViewer library, no need to call qmake and import the result.
Proper handling and versioning of dependencies. No hard-coded build paths showing up in unexpected places.