MantisBT - Doomseeker
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0003824Doomseeker[All Projects] Cleanuppublic2020-06-09 14:382022-05-31 01:43
ReporterWubTheCaptain 
Assigned ToBlzut3 
PrioritylowSeveritytrivialReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version1.3.1 
Target Version1.3.3Fixed in Version1.3.3 
Summary0003824: No AUTOUIC, qt5_wrap_ui() macro is used
DescriptionCMake'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.
TagsNo tags attached.
Relationships
child of 0003895needs testing Blzut3 Add support for Qt 6.2+ 
Attached Filespatch 0001-Reintroduce-cmake_policy-for-CMake-3.12.patch (1,856) 2021-08-10 10:02
https://zandronum.com/tracker/file_download.php?file_id=2655&type=bug
patch 0002-core-Bump-minimum-CMake-version-to-3.5.patch (1,211) 2021-08-10 10:02
https://zandronum.com/tracker/file_download.php?file_id=2656&type=bug
patch 0003-core-Suppress-CMake-CMP0028-warnings-again.patch (1,056) 2021-08-10 10:55
https://zandronum.com/tracker/file_download.php?file_id=2657&type=bug
Issue History
Date ModifiedUsernameFieldChange
2020-06-09 14:38WubTheCaptainNew Issue
2020-06-09 14:40WubTheCaptainNote Added: 0021460
2020-06-09 14:40WubTheCaptainSeverityfeature => trivial
2020-06-15 00:22Pol MNote Added: 0021463
2020-06-15 00:22Pol MStatusnew => confirmed
2021-08-08 21:49Blzut3Relationship addedchild of 0003895
2021-08-10 05:43WubTheCaptainPrioritynone => low
2021-08-10 07:46Blzut3Assigned To => Blzut3
2021-08-10 07:46Blzut3Statusconfirmed => assigned
2021-08-10 07:57Blzut3Note Added: 0021708
2021-08-10 07:57Blzut3Statusassigned => needs testing
2021-08-10 07:57Blzut3Target Version => 1.3.3
2021-08-10 09:12WubTheCaptainNote Added: 0021709
2021-08-10 09:12WubTheCaptainStatusneeds testing => needs review
2021-08-10 09:20WubTheCaptainNote Added: 0021710
2021-08-10 09:23Blzut3Note Added: 0021712
2021-08-10 09:38WubTheCaptainAssigned ToBlzut3 => WubTheCaptain
2021-08-10 09:38WubTheCaptainStatusneeds review => assigned
2021-08-10 09:46WubTheCaptainNote Added: 0021713
2021-08-10 10:02WubTheCaptainFile Added: 0001-Reintroduce-cmake_policy-for-CMake-3.12.patch
2021-08-10 10:02WubTheCaptainFile Added: 0002-core-Bump-minimum-CMake-version-to-3.5.patch
2021-08-10 10:03WubTheCaptainNote Added: 0021714
2021-08-10 10:03WubTheCaptainAssigned ToWubTheCaptain => Blzut3
2021-08-10 10:03WubTheCaptainStatusassigned => needs review
2021-08-10 10:03WubTheCaptainNote Edited: 0021714bug_revision_view_page.php?bugnote_id=21714#r13325
2021-08-10 10:18WubTheCaptainNote Added: 0021715
2021-08-10 10:26Blzut3Note Added: 0021716
2021-08-10 10:55WubTheCaptainFile Added: 0003-core-Suppress-CMake-CMP0028-warnings-again.patch
2021-08-10 17:55Blzut3Note Added: 0021720
2021-08-10 17:55Blzut3Note Edited: 0021720bug_revision_view_page.php?bugnote_id=21720#r13327
2021-08-15 02:44Blzut3Note Added: 0021725
2021-08-16 18:36WubTheCaptainRelationship addedparent of 0003900
2021-08-16 18:36WubTheCaptainRelationship deletedparent of 0003900
2022-03-23 02:27Blzut3Note Added: 0022155
2022-03-23 02:27Blzut3Statusneeds review => resolved
2022-03-23 02:27Blzut3Fixed in Version => 1.3.3
2022-03-23 02:27Blzut3Resolutionopen => fixed
2022-05-31 01:43WubTheCaptainStatusresolved => closed

Notes
(0021460)
WubTheCaptain   
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.
(0021463)
Pol M   
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.
(0021708)
Blzut3   
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 [^]'
(0021709)
WubTheCaptain   
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
If the running version of CMake is older than 3.12, the extra ... dots will be seen as version component separators, resulting in the ...<max> part being ignored and preserving the pre-3.12 behavior of basing policies on <min>.

'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
New in version 3.10.

Quote from CMP0071 — CMake 3.21.1 Documentation
CMake version 3.21.1 warns when the policy is not set and uses OLD behavior.

'https://cmake.org/cmake/help/v3.21/policy/CMP0071.html [^]'
(0021710)
WubTheCaptain   
2021-08-10 09:20   
Sorry, I had misunderstood or misread the cmake_minimum_required documentation.
(0021712)
Blzut3   
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.
(0021713)
WubTheCaptain   
2021-08-10 09:46   
What requires CMake 3.5? As far as I know, AUTOUIC was introduced in 3.0.
(0021714)
WubTheCaptain   
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.
(0021715)
WubTheCaptain   
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.
(0021716)
Blzut3   
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.
(0021720)
Blzut3   
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.
(0021725)
Blzut3   
2021-08-15 02:44   
Policy issues should be addressed with'https://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.
(0022155)
Blzut3   
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.