TDE
/
tdeutils
7
0
Fork 0
Code Issues 6 Pull Requests 2 Releases Wiki Activity
Labels Milestones

superkaramba: added cmake rules for example files and build xcursor.so at build time. #45

Merged
MicheleC merged 1 commits from superkaramba/examples-and-xcursor into master 1 year ago
Conversation 1 Commits 1 Files Changed 22
MicheleC commented 1 year ago
Owner

As per title.

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

As per title. On deb distros, to be built with TDE/tde-packaging#167.
MicheleC added this to the R14.1.0 release milestone 1 year ago
MicheleC added 1 commit 1 year ago
MicheleC requested review from SlavekB 1 year ago
SlavekB requested changes 1 year ago
SlavekB left a comment
Owner

Here are some comments and suggestions for simplification.

Here are some comments and suggestions for simplification.
ConfigureChecks.cmake Outdated
check_include_file( "sys/types.h" HAVE_SYS_TYPES_H )
find_package( PythonInterp )
find_package( PythonLibs )
SlavekB commented 1 year ago
Owner

A handy trick here is to reverse the order of detection – first the library, then the interpreter.

In the current order, the default python interpreter is detected – which on older distributions will be Python2, and then the latest python library is detected, which will be Python3. The result for building xcursor.so is FTBFS.
When the order is reversed, the python library detection causes the variables to be set so that the subsequent python interpreter detection returns the interpreter corresponding to the library.

In any case, the question here is whether interpreter detection will be needed – see another comment below.

A handy trick here is to reverse the order of detection – first the library, then the interpreter. In the current order, the default python interpreter is detected – which on older distributions will be Python2, and then the latest python library is detected, which will be Python3. The result for building `xcursor.so` is FTBFS. When the order is reversed, the python library detection causes the variables to be set so that the subsequent python interpreter detection returns the interpreter corresponding to the library. In any case, the question here is whether interpreter detection will be needed – see another comment below.
MicheleC commented 1 year ago
Poster
Owner

Confirmed that detecting the python interpreter is no longer needed with new cmake building code.

Confirmed that detecting the python interpreter is no longer needed with new cmake building code.
MicheleC marked this conversation as resolved
superkaramba/examples/CMakeLists.txt Outdated
INSTALL(
FILES API api.html README template.py test_all.sh
DESTINATION ${DATA_INSTALL_DIR}/superkaramba/examples/${_dir}
SlavekB commented 1 year ago
Owner

The variable _dir is not set here and therefore may contain an unknown value.
We normally use lowercase install.

The variable `_dir` is not set here and therefore may contain an unknown value. We normally use lowercase `install`.
MicheleC commented 1 year ago
Poster
Owner

No more applicable in new code.

No more applicable in new code.
MicheleC marked this conversation as resolved
superkaramba/examples/autoHide/CMakeLists.txt Outdated
endif( )
endif( )
install( FILES ${_file} DESTINATION ${DATA_INSTALL_DIR}/superkaramba/examples/${_curr_dir}${_subdir} )
endforeach()
SlavekB commented 1 year ago
Owner

Here are two things:

  1. The code seems too complex to simply copy a folder with its structure.
  2. It seems disadvantageous that the same code is repeated in each subfolder.

I suggest at the examples level to use the install with the signature for folders:

install(
  DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/
  DESTINATION ${DATA_INSTALL_DIR}/superkaramba/examples
  PATTERN CMakeLists.txt EXCLUDE
)

This should solve all the examples at once and avoid the need for CMakeLists.txt in each subdirectory.

Here are two things: 1. The code seems too complex to simply copy a folder with its structure. 2. It seems disadvantageous that the same code is repeated in each subfolder. I suggest at the `examples` level to use the `install` with the signature for folders: ``` install( DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/ DESTINATION ${DATA_INSTALL_DIR}/superkaramba/examples PATTERN CMakeLists.txt EXCLUDE ) ``` This should solve all the examples at once and avoid the need for `CMakeLists.txt` in each subdirectory.
MicheleC commented 1 year ago
Poster
Owner

Good suggestion, thanks.

Good suggestion, thanks.
MicheleC marked this conversation as resolved
superkaramba/examples/globalMouse/CMakeLists.txt Outdated
COMMAND ${PYTHON_EXECUTABLE} setup.py build
COMMAND find . -type f -name 'xcursor.*\.so' -exec cp {} xcursor.so "\;"
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
)
SlavekB commented 1 year ago
Owner

I believe that instead of creating a custom command with a python call to build the module, we can use our standard CMake rules:

include_directories(
  ${PYTHON_INCLUDE_DIR}
)

tde_add_library( xcursor MODULE NO_LIBTOOL_FILE
  SOURCES extension/xcursor.c
  LINK ${PYTHON_LIBRARIES} X11
  DESTINATION ${DATA_INSTALL_DIR}/superkaramba/examples/globalMouse
)

Because of this, there could be unnecessary detection of the Python interpreter added above.

I believe that instead of creating a custom command with a python call to build the module, we can use our standard CMake rules: ``` include_directories( ${PYTHON_INCLUDE_DIR} ) tde_add_library( xcursor MODULE NO_LIBTOOL_FILE SOURCES extension/xcursor.c LINK ${PYTHON_LIBRARIES} X11 DESTINATION ${DATA_INSTALL_DIR}/superkaramba/examples/globalMouse ) ``` Because of this, there could be unnecessary detection of the Python interpreter added above.
MicheleC commented 1 year ago
Poster
Owner

Nice, this also solves the issue that with Python 3.12 distutils would be removed.

Nice, this also solves the issue that with Python 3.12 `distutils` would be removed.
MicheleC marked this conversation as resolved
superkaramba/examples/test_all.sh Outdated
"graph/graph.theme"
"image/image.theme"
"input_api/input_api.theme"
"input_example/input_example.theme"
SlavekB commented 1 year ago
Owner

Inconsistent indentation – tabs × spaces.

Inconsistent indentation – tabs × spaces.
MicheleC commented 1 year ago
Poster
Owner

Fixed.

Fixed.
MicheleC marked this conversation as resolved
MicheleC force-pushed superkaramba/examples-and-xcursor from a7a4d432c4 to 09d30bd11a 1 year ago
MicheleC requested review from SlavekB 1 year ago
MicheleC commented 1 year ago
Poster
Owner

New version ready for another review.

New version ready for another review.
SlavekB requested changes 1 year ago
SlavekB left a comment
Owner

It looks good. Unfortunately, I noticed another little thing – see the comment below.

It looks good. Unfortunately, I noticed another little thing – see the comment below.
superkaramba/examples/test_all.sh
#!/bin/sh
SlavekB commented 1 year ago
Owner

We should require /bin/bash instead of /bin/sh, because the variable with the array is bashism, so it could report a syntax error if the default shell is different than bash.

We should require `/bin/bash` instead of `/bin/sh`, because the variable with the array is _bashism_, so it could report a syntax error if the default shell is different than bash.
SlavekB commented 1 year ago
Owner

Another option, probably better, would be to avoid array:

themes="
  autoHide/main.theme
  bar/bar.theme
  change_interval/interval.theme
  .
  .
  .
  text/text.theme
  unicode/unicode.theme
"

for theme in $themes
do
Another option, probably better, would be to avoid array: ``` themes=" autoHide/main.theme bar/bar.theme change_interval/interval.theme . . . text/text.theme unicode/unicode.theme " for theme in $themes do ```
MicheleC commented 1 year ago
Poster
Owner

Updated following option 2.

Updated following option 2.
MicheleC marked this conversation as resolved
MicheleC force-pushed superkaramba/examples-and-xcursor from 09d30bd11a to eef19dd535 1 year ago
MicheleC requested review from SlavekB 1 year ago
SlavekB approved these changes 1 year ago
SlavekB left a comment
Owner

Now everything looks good.

Now everything looks good.
MicheleC merged commit eef19dd535 into master 1 year ago
MicheleC deleted branch superkaramba/examples-and-xcursor 1 year ago

Reviewers

SlavekB approved these changes 1 year ago
The pull request has been merged as eef19dd535.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
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.0 release
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Blocks
#167 DEB tdeutils: updated to new cmake code for superkaramba.
TDE/tde-packaging
Reference: TDE/tdeutils#45
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'superkaramba/examples-and-xcursor'

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

No
Yes