Anonymous | Login | Signup for a new account | 2023-03-29 07:11 UTC | ![]() |
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account |
View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
0003824 | Doomseeker | [All Projects] Cleanup | public | 2020-06-09 14:38 | 2022-05-31 01:43 | ||||
Reporter | WubTheCaptain | ||||||||
Assigned To | Blzut3 | ||||||||
Priority | low | Severity | trivial | Reproducibility | always | ||||
Status | closed | Resolution | fixed | ||||||
Platform | OS | OS Version | |||||||
Product Version | 1.3.1 | ||||||||
Target Version | 1.3.3 | Fixed in Version | 1.3.3 | ||||||
Summary | 0003824: No AUTOUIC, qt5_wrap_ui() macro is used | ||||||||
Description | CMake's build files handle Qt's XML format user interface definition (.ui) files manually, with qt5_wrap_ui() macro. May be more ideal to use CMake's AUTOUIC to avoid cluttering the build files, which has been supported since CMake 3.0 or so. | ||||||||
Steps To Reproduce | $ grep -r "qt5_wrap_ui" . ./src/plugins/zandronum/CMakeLists.txt:qt5_wrap_ui(ZANDRONUM_UI_FILES ./src/core/CMakeLists.txt:qt5_wrap_ui(doomseekerUI ${UI_FILES}) $ grep -r "UI_FILES" . ./src/plugins/zandronum/CMakeLists.txt:qt5_wrap_ui(ZANDRONUM_UI_FILES ./src/plugins/zandronum/CMakeLists.txt: ${ZANDRONUM_UI_FILES} ./src/plugins/zandronum/CMakeLists.txt: ${ZANDRONUM_UI_FILES} ./src/core/CMakeSpawnFileListing.cmake:file(GLOB_RECURSE UI_FILES RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} "*.ui") ./src/core/CMakeSpawnFileListing.cmake:append_file_list(${FILE_NAME} "UI_FILES" "${UI_FILES}") ./src/core/CMakeLists.txt:qt5_wrap_ui(doomseekerUI ${UI_FILES}) ./src/core/CMakeLists.txt: ${UI_FILES} ./src/core/CMakeFileListing.cmake:set(UI_FILES | ||||||||
Additional Information | https://cmake.org/cmake/help/latest/prop_tgt/AUTOUIC.html [^] https://stackoverflow.com/a/42331649 [^] git blame src/core/CMakeLists.txt says git show 95cdd026d, by Pol M. | ||||||||
Attached Files | ![]() ![]() ![]() | ||||||||
![]() |
|
WubTheCaptain (reporter) 2020-06-09 14:40 |
And no offense to Pol M, the previous qt4_wrap_ui() that commit mentioned in OP was likely predating CMake 3.0 too. |
Pol M (developer) 2020-06-15 00:22 |
Pretty in favor, I did not know this existed and it seems cleaner and more future proof. Any shenanigans that we do in cmake that can be automated further are a clear improvement. No offense taken, I just took the simple path as at the time that was as much as it was needed. |
Blzut3 (administrator) 2021-08-10 07:57 |
Enabled AUTOUIC:https://bitbucket.org/Doomseeker/doomseeker/commits/d6ec9cc30dc5373835511db1b15703b5491f6fbb [^] Then noticed AUTORCC is a thing too so:https://bitbucket.org/Doomseeker/doomseeker/commits/952afc8de4b2854761348055b04b202276c83aef [^] |
WubTheCaptain (reporter) 2021-08-10 09:12 |
Quote-cmake_minimum_required(VERSION 2.8.12) +cmake_minimum_required(VERSION 3.5...3.21) Quote from cmake_minimum_required — CMake 3.21.1 Documentation https://cmake.org/cmake/help/v3.21/command/cmake_minimum_required.html [^] Should that be cmake_minimum_required(VERSION 3.5)instead? Quote-if (POLICY CMP0071) - cmake_policy(SET CMP0071 NEW) -endif() Quote from CMP0071 — CMake 3.21.1 Documentation Quote from CMP0071 — CMake 3.21.1 Documentation https://cmake.org/cmake/help/v3.21/policy/CMP0071.html [^] |
WubTheCaptain (reporter) 2021-08-10 09:20 |
Sorry, I had misunderstood or misread the cmake_minimum_required documentation. |
Blzut3 (administrator) 2021-08-10 09:23 |
Yeah the point of using a range is so that you can keep the minimum low while having all the new policies set to NEW automatically. I suppose you're correct that 3.10 and 3.11 would raise a warning on that though. |
WubTheCaptain (reporter) 2021-08-10 09:46 |
What requires CMake 3.5? As far as I know, AUTOUIC was introduced in 3.0. |
WubTheCaptain (reporter) 2021-08-10 10:03 edited on: 2021-08-10 10:03 |
Maybe the attached patches 0001 & 0002 address the notes from the previous code review. But I didn't compile this. |
WubTheCaptain (reporter) 2021-08-10 10:18 |
Also fyi, using AUTOUIC means we no longer support Ubuntu 14.04 LTS (which I believe only has extended security maintenance until April 2022, and has ended standard support already), which ships with CMake 2.8.12. Ubuntu 16.04 LTS is not affected - it has CMake 3.5. |
Blzut3 (administrator) 2021-08-10 10:26 |
Can't comment on the patches right now, but as far as the 3.5 requirement that comes from practicality. This change requires 3.0, going to 3.1 allows dropping some legacy junk. It's not impossible that 3.1 could be the new literal minimum, but why spend cycles figuring that out? We only test for 3.5+ since that's what our CI container has. Given that we only really support Ubuntu 18.04+ and similar era distributions at this point (per policy of following Canonical's free support period) we really could bump it to 3.10, so 3.5 is being plenty generous I think. |
Blzut3 (administrator) 2021-08-10 17:55 edited on: 2021-08-10 17:55 |
Patch 0001 - I believe we can just use cmake_policy(VERSION 3.21) with a comment that it can be removed after the minimum becomes 3.12. Would need to test. Patch 0002 - The line can just be removed. I thought I did remove it, but I guess not. Patch 0003 - That policy was introduced in 3.0 so even if the minimum was set to that this policy should always be NEW anyway. |
Blzut3 (administrator) 2021-08-15 02:44 |
Policy issues should be addressed withhttps://bitbucket.org/Doomseeker/doomseeker/commits/5c20503430cc845bfaa7273b21475f036c50123d [^] Turns out cmake_policy(VERSION) will reject versions greater than CMAKE_VERSION, but I suppose there's no reason we can't feed CMAKE_VERSION into it if we're running on a version of CMake less than 3.12 and effectively emulate the range behavior. |
Blzut3 (administrator) 2022-03-23 02:27 |
Going to consider this one resolved since we managed a release and it doesn't seem any issues came from it. |
This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why. |
|
Supporters: | Zalewa Pol M |
Opponents: | No one explicitly opposes this issue yet. |
![]() |
|||
Date Modified | Username | Field | Change |
2020-06-09 14:38 | WubTheCaptain | New Issue | |
2020-06-09 14:40 | WubTheCaptain | Note Added: 0021460 | |
2020-06-09 14:40 | WubTheCaptain | Severity | feature => trivial |
2020-06-15 00:22 | Pol M | Note Added: 0021463 | |
2020-06-15 00:22 | Pol M | Status | new => confirmed |
2021-08-08 21:49 | Blzut3 | Relationship added | child of 0003895 |
2021-08-10 05:43 | WubTheCaptain | Priority | none => low |
2021-08-10 07:46 | Blzut3 | Assigned To | => Blzut3 |
2021-08-10 07:46 | Blzut3 | Status | confirmed => assigned |
2021-08-10 07:57 | Blzut3 | Note Added: 0021708 | |
2021-08-10 07:57 | Blzut3 | Status | assigned => needs testing |
2021-08-10 07:57 | Blzut3 | Target Version | => 1.3.3 |
2021-08-10 09:12 | WubTheCaptain | Note Added: 0021709 | |
2021-08-10 09:12 | WubTheCaptain | Status | needs testing => needs review |
2021-08-10 09:20 | WubTheCaptain | Note Added: 0021710 | |
2021-08-10 09:23 | Blzut3 | Note Added: 0021712 | |
2021-08-10 09:38 | WubTheCaptain | Assigned To | Blzut3 => WubTheCaptain |
2021-08-10 09:38 | WubTheCaptain | Status | needs review => assigned |
2021-08-10 09:46 | WubTheCaptain | Note Added: 0021713 | |
2021-08-10 10:02 | WubTheCaptain | File Added: 0001-Reintroduce-cmake_policy-for-CMake-3.12.patch | |
2021-08-10 10:02 | WubTheCaptain | File Added: 0002-core-Bump-minimum-CMake-version-to-3.5.patch | |
2021-08-10 10:03 | WubTheCaptain | Note Added: 0021714 | |
2021-08-10 10:03 | WubTheCaptain | Assigned To | WubTheCaptain => Blzut3 |
2021-08-10 10:03 | WubTheCaptain | Status | assigned => needs review |
2021-08-10 10:03 | WubTheCaptain | Note Edited: 0021714 | View Revisions |
2021-08-10 10:18 | WubTheCaptain | Note Added: 0021715 | |
2021-08-10 10:26 | Blzut3 | Note Added: 0021716 | |
2021-08-10 10:55 | WubTheCaptain | File Added: 0003-core-Suppress-CMake-CMP0028-warnings-again.patch | |
2021-08-10 17:55 | Blzut3 | Note Added: 0021720 | |
2021-08-10 17:55 | Blzut3 | Note Edited: 0021720 | View Revisions |
2021-08-15 02:44 | Blzut3 | Note Added: 0021725 | |
2021-08-16 18:36 | WubTheCaptain | Relationship added | parent of 0003900 |
2021-08-16 18:36 | WubTheCaptain | Relationship deleted | parent of 0003900 |
2022-03-23 02:27 | Blzut3 | Note Added: 0022155 | |
2022-03-23 02:27 | Blzut3 | Status | needs review => resolved |
2022-03-23 02:27 | Blzut3 | Fixed in Version | => 1.3.3 |
2022-03-23 02:27 | Blzut3 | Resolution | open => fixed |
2022-05-31 01:43 | WubTheCaptain | Status | resolved => closed |
Copyright © 2000 - 2023 MantisBT Team |