TDE
/
gwenview
6
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

Labels Milestones

Conversion to cmake building system #30

Merged
MicheleC merged 1 commits from feat/cmake-conversion into master 12 months ago
Conversation 10 Commits 1 Files Changed 19
MicheleC commented 12 months ago
Owner

As per title.

On deb distros, to be built with TDE/tde-packaging#280.

As per title. On deb distros, to be built with TDE/tde-packaging#280.
MicheleC added this to the R14.1.2 release milestone 12 months ago
MicheleC added 1 commit 12 months ago
1857981b8c
Conversion to cmake building system 
Signed-off-by: Michele Calgaro <michele.calgaro@yahoo.it>
MicheleC requested review from Fat-Zer 12 months ago
MicheleC requested review from Core 12 months ago
MicheleC requested review from Owners 12 months ago
Fat-Zer commented 12 months ago
Collaborator

At least kipi plugins support should be optional.

Also it makes sense to make optional (in order of usefulness of the options):

  • mng
  • jpeg
  • xcursors
At least kipi plugins support should be optional. Also it makes sense to make optional (in order of usefulness of the options): - mng - jpeg - xcursors
MicheleC commented 12 months ago
Poster
Owner

Good point on kipi that should be optional, I missed that.
I can also work to add conditional support for the other 3 libs, if that is beneficial.

Good point on kipi that should be optional, I missed that. I can also work to add conditional support for the other 3 libs, if that is beneficial.
Fat-Zer commented 12 months ago
Collaborator

I can also work to add conditional support for the other 3 libs, if that is beneficial.

mng is a rather rare beast; it'd be nice to have it optional. jpeg and xcursors are more common, so those are not that pressing, but I believe making them optional won't hurt either.

PS: on a side note does debian provides a *.pc file for mng?
PPS: On a side note, all this formats are also directly supported by TQt, so I'm not that sure what gwenview does to improve upon that...

> I can also work to add conditional support for the other 3 libs, if that is beneficial. mng is a rather rare beast; it'd be nice to have it optional. jpeg and xcursors are more common, so those are not that pressing, but I believe making them optional won't hurt either. PS: on a side note does debian provides a `*.pc` file for mng? PPS: On a side note, all this formats are also directly supported by TQt, so I'm not that sure what gwenview does to improve upon that...
MicheleC commented 12 months ago
Poster
Owner

mng is a rather rare beast; it'd be nice to have it optional. jpeg and xcursors are more common, so those are not that pressing, but I believe making them optional won't hurt either.

Sure, no prob.

PS: on a side note does debian provides a *.pc file for mng?

Doesn't seem like it

> mng is a rather rare beast; it'd be nice to have it optional. jpeg and xcursors are more common, so those are not that pressing, but I believe making them optional won't hurt either. Sure, no prob. > PS: on a side note does debian provides a *.pc file for mng? Doesn't seem like it
Fat-Zer reviewed 12 months ago
src/CMakeLists.txt
@ -0,0 +8,4 @@
link_directories(
${TDE_LIB_DIR}
)
Fat-Zer commented 12 months ago
Collaborator

Do we need all this both here and in all the subdirectories?

Do we need all this both here and in all the subdirectories?
MicheleC commented 12 months ago
Poster
Owner

I can test if some/all can be removed. I sort of use a template across the folders to have thinks look more similar. But it is a valid point.

I can test if some/all can be removed. I sort of use a template across the folders to have thinks look more similar. But it is a valid point.
MicheleC commented 12 months ago
Poster
Owner

Done

Done
Fat-Zer commented 12 months ago
Collaborator

The include_directories() above are also redundant...

The `include_directories()` above are also redundant...
Fat-Zer commented 12 months ago
Collaborator

And as for link_directories() I meant it's needed either here or in the subdirectories; at least in one, but not in both.

And as for `link_directories()` I meant it's needed either here or in the subdirectories; at least in one, but not in both.
MicheleC commented 12 months ago
Poster
Owner

Re link directories, they are included in one cmake file and I can build gwenview fine (while removing the others, I tested that the build would still succeed).
Re includes, I guess we could do some removal too. In general, we should probably consider this as a TDE wide task, since many cmake conversions have a very similar structure for includes and links.

Re link directories, they are included in one cmake file and I can build gwenview fine (while removing the others, I tested that the build would still succeed). Re includes, I guess we could do some removal too. In general, we should probably consider this as a TDE wide task, since many cmake conversions have a very similar structure for includes and links.
MicheleC commented 12 months ago
Poster
Owner

ah I see what you mean. It was for that specific src/CMakeLists.txt file right? yes, they are not required. I will update again.

ah I see what you mean. It was for that specific `src/CMakeLists.txt` file right? yes, they are not required. I will update again.
MicheleC commented 12 months ago
Poster
Owner

I have cleaned up includes and link directories, adding the common ones in src/CMakeLists.txt and removing the duplicates from all the other files.

I have cleaned up includes and link directories, adding the common ones in `src/CMakeLists.txt` and removing the duplicates from all the other files.
👍 1
Fat-Zer commented 12 months ago
Collaborator

Looks good.

If I were neat-picking, I'd say that ${CMAKE_CURRENT_SOURCE_DIR} and ${CMAKE_CURRENT_BINARY_DIR} in src/CMakeLists.txt are also redundant ;)

Looks good. If I were neat-picking, I'd say that `${CMAKE_CURRENT_SOURCE_DIR}` and `${CMAKE_CURRENT_BINARY_DIR}` in `src/CMakeLists.txt` are also redundant ;)
SlavekB commented 12 months ago
Owner

If I were neat-picking, I'd say that ${CMAKE_CURRENT_SOURCE_DIR} and ${CMAKE_CURRENT_BINARY_DIR} in src/CMakeLists.txt are also redundant ;)

I would say they are not redundant, but unnecessary in this folder because it does not contain any source or generated headers.

> If I were neat-picking, I'd say that `${CMAKE_CURRENT_SOURCE_DIR}` and `${CMAKE_CURRENT_BINARY_DIR}` in `src/CMakeLists.txt` are also redundant ;) I would say they are not _redundant_, but _unnecessary_ in this folder because it does not contain any source or generated headers.
MicheleC commented 12 months ago
Poster
Owner

If I were neat-picking, I'd say that {CMAKE_CURRENT_SOURCE_DIR} and {CMAKE_CURRENT_BINARY_DIR} in src/CMakeLists.txt are also redundant ;)

${CMAKE_CURRENT_BINARY_DIR} is unnecessary (I have removed that), but ${CMAKE_CURRENT_SOURCE_DIR} is required because several files include files as #include "imageutils/imageutils.h", with refers to the src folder (whic is ${CMAKE_CURRENT_SOURCE_DIR}). Alternatively we would have to modify all the includes and make sure we still build fine with autotools too.

> If I were neat-picking, I'd say that ${CMAKE_CURRENT_SOURCE_DIR} and ${CMAKE_CURRENT_BINARY_DIR} in src/CMakeLists.txt are also redundant ;) `${CMAKE_CURRENT_BINARY_DIR}` is unnecessary (I have removed that), but `${CMAKE_CURRENT_SOURCE_DIR}` is required because several files include files as `#include "imageutils/imageutils.h"`, with refers to the `src` folder (whic is `${CMAKE_CURRENT_SOURCE_DIR}`). Alternatively we would have to modify all the includes and make sure we still build fine with autotools too.
Fat-Zer marked this conversation as resolved
src/imageutils/CMakeLists.txt
@ -0,0 +28,4 @@
tde_add_check_executable( testjpegcontent AUTOMOC
SOURCES testjpegcontent.cpp
LINK gvimageutils-static
)
Fat-Zer commented 12 months ago
Collaborator

This one seems to be an automated test, if so, you may add TEST, so in get run by make test

This one seems to be an automated test, if so, you may add `TEST`, so in get run by `make test`
MicheleC commented 12 months ago
Poster
Owner

Not really, IMO. Inside it has a TDE Application (which implicitly has a GUI and will bring up Dr. Konqui in case of crash) and at the end it prints some errors and a message to the user saying that he will see some error on the console.

It is more a test to be run manually IMO, which is why I didn't add TEST.

Not really, IMO. Inside it has a TDE Application (which implicitly has a GUI and will bring up Dr. Konqui in case of crash) and at the end it prints some errors and a message to the user saying that he will see some error on the console. It is more a test to be run manually IMO, which is why I didn't add `TEST`.
Fat-Zer commented 12 months ago
Collaborator

yep... it seems you are right...

yep... it seems you are right...
Fat-Zer marked this conversation as resolved
Fat-Zer commented 12 months ago
Collaborator

Doesn't seem like it

Ok, thanks, I'll probably have to revert some changes in tqt then

> Doesn't seem like it Ok, thanks, I'll probably have to revert some changes in tqt then
MicheleC commented 12 months ago
Poster
Owner

There is no conditional code related to exiv2, jpeg and png. If we make them optional, we would have link failures unless we add #ifdef somewhere in the code. Therefore I will make kipi, mng and xcursor optional but not exiv2, jpeg and png.
What do you think?

There is no conditional code related to exiv2, jpeg and png. If we make them optional, we would have link failures unless we add #ifdef somewhere in the code. Therefore I will make kipi, mng and xcursor optional but not exiv2, jpeg and png. What do you think?
MicheleC force-pushed feat/cmake-conversion from 1857981b8c to aec60d6567 12 months ago
MicheleC commented 12 months ago
Poster
Owner

PR updated with the conditional explained above and removing most of the linking directories.
Still pending the decision on the asm file (see PR #29).

PR updated with the conditional explained above and removing most of the linking directories. Still pending the decision on the asm file (see PR #29).
Fat-Zer commented 12 months ago
Collaborator

There is no conditional code related to exiv2, jpeg and png. If we make them optional, we would have link failures unless we add #ifdef somewhere in the code. Therefore I will make kipi, mng and xcursor optional but not exiv2, jpeg and png.
What do you think?

that's fine by me...

there is an issue: link_directories() is actually needed, see comment above

>There is no conditional code related to exiv2, jpeg and png. If we make them optional, we would have link failures unless we add #ifdef somewhere in the code. Therefore I will make kipi, mng and xcursor optional but not exiv2, jpeg and png. What do you think? that's fine by me... there is an issue: `link_directories()` is actually needed, see comment above
MicheleC force-pushed feat/cmake-conversion from aec60d6567 to 59c454cca8 12 months ago
MicheleC commented 12 months ago
Poster
Owner

I have updated the PR to include the asm file in ix86 builds and make sure that HAVE_X86_MMX is defined in such case if support for MMX is detected.

there is an issue: link_directories() is actually needed, see comment above

see comment in above subdiscussion.

I have updated the PR to include the asm file in ix86 builds and make sure that `HAVE_X86_MMX` is defined in such case if support for MMX is detected. > there is an issue: link_directories() is actually needed, see comment above see comment in above subdiscussion.
MicheleC force-pushed feat/cmake-conversion from 59c454cca8 to 2410e28ae8 12 months ago
SlavekB requested changes 12 months ago
SlavekB left a comment
Owner

Here are some comments – see below.

Here are some comments – see below.
ConfigureChecks.cmake
@ -0,0 +47,4 @@
##### checks for libkipi
if( WITH_KIPI )
find_file( HAVE_LIBKIPI_H "libkipi/interface.h" )
SlavekB commented 12 months ago
Owner

For libkipi we have a file for pkg-config, so instead of searching for a header file we can use pkg-config detection.

For `libkipi` we have a file for pkg-config, so instead of searching for a header file we can use pkg-config detection.
MicheleC marked this conversation as resolved
icons/cursor/CMakeLists.txt
@ -0,0 +1,4 @@
INSTALL(
SlavekB commented 12 months ago
Owner

We do not use install written in capital letters.

We do not use `install` written in capital letters.
MicheleC commented 12 months ago
Poster
Owner

There are lots of INSTALL in capital letters in TDE cmake files, jsut run rg -g CMake* "INSTALL\(" -l from the tde/main folder.
I have no problem to make it lower case, just pointing out that if we want to make that the standard, we should also update those other files. I can do that too, rather quickly actually, if needed.

There are lots of `INSTALL` in capital letters in TDE cmake files, jsut run `rg -g CMake* "INSTALL\(" -l` from the tde/main folder. I have no problem to make it lower case, just pointing out that if we want to make that the standard, we should also update those other files. I can do that too, rather quickly actually, if needed.
MicheleC commented 12 months ago
Poster
Owner

I have renamed to lower letters (will push commit soon). Let me know if the same is preferrable in all the others usages in TDE

I have renamed to lower letters (will push commit soon). Let me know if the same is preferrable in all the others usages in TDE
SlavekB commented 12 months ago
Owner

We certainly prefer it. The question is whether to do this as a separate mass commit or do it gradually with the introduction of the rules for code formatting.

We certainly prefer it. The question is whether to do this as a separate mass commit or do it gradually with the introduction of the rules for code formatting.
MicheleC commented 12 months ago
Poster
Owner

I can do a selected mass renaming tomorrow, it won't take long.
Code formatting would be an independent topic.

I can do a selected mass renaming tomorrow, it won't take long. Code formatting would be an independent topic.
MicheleC marked this conversation as resolved
icons/thumbnail/CMakeLists.txt
@ -0,0 +1,4 @@
INSTALL(
SlavekB commented 12 months ago
Owner

We do not use install written in capital letters.

We do not use `install` written in capital letters.
MicheleC marked this conversation as resolved
src/gvcore/CMakeLists.txt
@ -0,0 +74,4 @@
tdecore-shared tdeio-shared tdemediaplayer-shared
tdeparts-shared tdeprint-shared tdeui-shared
${EXIV2_LIBRARIES} ${JPEG_LIBRARIES} ${MNG_LIBRARY}
${PNG_LIBRARIES} ${XCURSOR_LIBRARIES}
SlavekB commented 12 months ago
Owner

mismatch in indentation

mismatch in indentation
MicheleC marked this conversation as resolved
src/imageutils/CMakeLists.txt
@ -0,0 +18,4 @@
set_source_files_properties( asm_scale.S PROPERTIES LANGUAGE C )
if( HAVE_X86_MMX )
add_compile_options( -DHAVE_X86_MMX )
SlavekB commented 12 months ago
Owner

It seems that it is possible to combine the processor architecture condition along with the HAVE_X86_MMX condition, because otherwise there is no point in adding the source file to the list. In addition, HAVE_X86_MMX will never be set unless processor architecture is i?86, so the architecture test is unnecessary – the HAVE_X86_MMX condition is sufficient.

It seems that it is possible to combine the processor architecture condition along with the `HAVE_X86_MMX` condition, because otherwise there is no point in adding the source file to the list. In addition, `HAVE_X86_MMX` will never be set unless processor architecture is i?86, so the architecture test is unnecessary – the `HAVE_X86_MMX` condition is sufficient.
MicheleC marked this conversation as resolved
MicheleC force-pushed feat/cmake-conversion from 2410e28ae8 to 654e9023d1 12 months ago
MicheleC commented 12 months ago
Poster
Owner

PR updated based on the feedback.

PR updated based on the feedback.
MicheleC requested review from SlavekB 12 months ago
MicheleC force-pushed feat/cmake-conversion from 654e9023d1 to 236e2dc3c6 12 months ago
Fat-Zer approved these changes 12 months ago
Fat-Zer left a comment
Collaborator

LGTM

LGTM
SlavekB approved these changes 12 months ago
SlavekB left a comment
Owner

I didn't test, but all looks good.

I didn't test, but all looks good.
MicheleC force-pushed feat/cmake-conversion from 236e2dc3c6 to bc9bda10c9 12 months ago
MicheleC merged commit bc9bda10c9 into master 12 months ago
MicheleC deleted branch feat/cmake-conversion 12 months ago

Reviewers

Fat-Zer approved these changes 12 months ago
SlavekB approved these changes 12 months ago
TDE/Core was requested for review 12 months ago
The pull request has been merged as bc9bda10c9.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Fat-Zer
SlavekB
TDE/Core
Labels
Apply labels
Clear labels   
GE/need-info

General - need additional info from contributor   
PR/keep-branch

Pull request - do not delete branch after merging   
PR/not-ok

Pull request - need fixing   
PR/rfc

Pull request - request for comments   
PR/update-trans

Pull request - update to translation files needed   
PR/wip

Pull request - work in progress   
RS/R14.0.x

Related to R14.0.x series   
RS/R14.1.x

Related to R14.1.x series   
SL/critical

Severity level - critical   
SL/major

Severity level - major   
SL/minor

Severity level - minor   
SL/normal

Severity level - normal   
SL/regression

Severity level - regression from previous version   
SL/trivial

Severity level - trivial   
SL/wishlist

Severity level - wishlist request   
ST/duplicate

Status - duplicate of another issue   
ST/invalid

Status - invalid report   
ST/notourproblem

Status - not our problem   
ST/rejected

Status - rejected   
ST/wontfix

Status - won't fix   
ST/worksforme

Status - works for me, unable to reproduce
No Label
GE/need-info
PR/keep-branch
PR/not-ok
PR/rfc
PR/update-trans
PR/wip
RS/R14.0.x
RS/R14.1.x
SL/critical
SL/major
SL/minor
SL/normal
SL/regression
SL/trivial
SL/wishlist
ST/duplicate
ST/invalid
ST/notourproblem
ST/rejected
ST/wontfix
ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.1.2 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Blocks
#280 DEB gwenview: switch to cmake build system
TDE/tde-packaging
Reference: TDE/gwenview#30
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feat/cmake-conversion'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes