superkaramba: fixed SEGV when loading python scripts. #44

Merged
MicheleC merged 2 commits from fix/issue/43/superkaramba-segv into master 1 year ago
Owner

Needs testing with python < 3.7 due to changes in API from version 3.7.
This resolves issue #43.

Needs testing with python < 3.7 due to changes in API from version 3.7. This resolves issue #43.
MicheleC added 1 commit 1 year ago
76bfc28b3f
superkaramba: fixed SEGV when loading python scripts.
MicheleC requested review from SlavekB 1 year ago
MicheleC added this to the R14.0.13 release milestone 1 year ago
MicheleC force-pushed fix/issue/43/superkaramba-segv from 76bfc28b3f to c465f60e58 1 year ago
SlavekB commented 1 year ago
Owner

I tried to build on Debian 9.x (Stretch) – that's Python 3.5.x and it leads to FTBFS:

superkaramba/src/karamba_python.cpp:485:17: error: 'Py_FinalizeEx' was not declared in this scope
   Py_FinalizeEx();
                 ^
I tried to build on Debian 9.x (Stretch) – that's Python 3.5.x and it leads to FTBFS: ``` superkaramba/src/karamba_python.cpp:485:17: error: 'Py_FinalizeEx' was not declared in this scope Py_FinalizeEx(); ^ ```
MicheleC force-pushed fix/issue/43/superkaramba-segv from c465f60e58 to f61073a409 1 year ago
Poster
Owner

I pushed an updated version. Py_FinalizeEx is not available prior to 3.6, so I reverted to the original Py_Finalize form.

I pushed an updated version. `Py_FinalizeEx` is not available prior to 3.6, so I reverted to the original `Py_Finalize` form.
SlavekB commented 1 year ago
Owner

I pushed an updated version. Py_FinalizeEx is not available prior to 3.6, so I reverted to the original Py_Finalize form.

Great, now build is successful also on Python 3.5.x.

> I pushed an updated version. `Py_FinalizeEx` is not available prior to 3.6, so I reverted to the original `Py_Finalize` form. Great, now build is successful also on Python 3.5.x.
Poster
Owner

Comparing with KDE 3.5.10 on Lenny with python 2, it looks like this fix is not enough. Back to the workboard ;-)

Comparing with KDE 3.5.10 on Lenny with python 2, it looks like this fix is not enough. Back to the workboard ;-)
MicheleC changed title from superkaramba: fixed SEGV when loading python scripts. to WIP: superkaramba: fixed SEGV when loading python scripts. 1 year ago
MicheleC modified the milestone from R14.0.13 release to R14.1.0 release 1 year ago
MicheleC force-pushed fix/issue/43/superkaramba-segv from f61073a409 to 5dab6232ef 1 year ago
Poster
Owner

I have pushed an updated commit that seems to solve the issues with superkaramba and python3. Python scripts in examples will need to be modified from python 2 to python 3, but I will work on those in coming days. In the meantime it would be good to test this in other older distros to see if there is any issue with it.

I have pushed an updated commit that seems to solve the issues with superkaramba and python3. Python scripts in examples will need to be modified from python 2 to python 3, but I will work on those in coming days. In the meantime it would be good to test this in other older distros to see if there is any issue with it.
MicheleC added 1 commit 1 year ago
9cd8533e12
[temp commit] superkaramba: convert examples to python3.
Poster
Owner

Uploaded first batch of converted examples to help testing.

Uploaded first batch of converted examples to help testing.
MicheleC force-pushed fix/issue/43/superkaramba-segv from 9cd8533e12 to 3ac0a96fbb 1 year ago
MicheleC changed title from WIP: superkaramba: fixed SEGV when loading python scripts. to superkaramba: fixed SEGV when loading python scripts. 1 year ago
Poster
Owner

Examples converted to python3.
PR ready for review and merge.

Examples converted to python3. PR ready for review and merge.
SlavekB reviewed 1 year ago
SlavekB left a comment
Owner

Below are some notes for checking and decisions.

In addition, in the examples/autoHide/main.py file is the occurrence of print "Loaded my python extension!" that needs adjustment for compatibility with Python3. The same would be good to do also in examples/template.py.

Next to this here is the idea of adding a build of xcursor.so to CMake rules to ensure that it will be ready for the current system.

Below are some notes for checking and decisions. In addition, in the `examples/autoHide/main.py` file is the occurrence of `print "Loaded my python extension!"` that needs adjustment for compatibility with Python3. The same would be good to do also in `examples/template.py`. Next to this here is the idea of adding a build of `xcursor.so` to CMake rules to ensure that it will be ready for the current system.
pass
#This gets called everytime our widget is clicked.
#Notes
SlavekB commented 1 year ago
Owner

Is there any reason that this comment is reduced? I know that in this particular example of values are not essential, but I assume that the comment here fulfills the educational purpose.

Is there any reason that this comment is reduced? I know that in this particular example of values are not essential, but I assume that the comment here fulfills the educational purpose.
Poster
Owner

That comment was wrong, it referred to the mouseClick event which is above this comment. You can see at line 22 that there is the same comment.
Instead at line 36 there is the mouseMove event, so I have updated the comment to refer to the right event.

That comment was wrong, it referred to the `mouseClick` event which is above this comment. You can see at line 22 that there is the same comment. Instead at line 36 there is the `mouseMove` event, so I have updated the comment to refer to the right event.
SlavekB commented 1 year ago
Owner

Oh, I understand, I didn't look at it in detail. So your adjustment makes sense.

Oh, I understand, I didn't look at it in detail. So your adjustment makes sense.
MicheleC marked this conversation as resolved
#print xp, yp
xp = xp - rp_width // 2
yp = yp - rp_height // 2
#print xp, yp
SlavekB commented 1 year ago
Owner

Although these occurrences are in the note, it would be good for the code to be updated for compatibility with Python3. Here are more such cases in this file.

Although these occurrences are in the note, it would be good for the code to be updated for compatibility with Python3. Here are more such cases in this file.
Poster
Owner

Good point, those were not converted automatically by the 2to3 script. I have checked also the other examples.

Good point, those were not converted automatically by the 2to3 script. I have checked also the other examples.
MicheleC marked this conversation as resolved
for image in taskPanels:
karamba.hideImage(widget, image)
#for image in taskPanels:
# karamba.hideImage(widget, image)
SlavekB commented 1 year ago
Owner

I didn't do a test, but I am surprised that this code is postponed?

I didn't do a test, but I am surprised that this code is postponed?
Poster
Owner

ah, I forgot to uncomment this. My bad.

ah, I forgot to uncomment this. My bad.
MicheleC marked this conversation as resolved
MicheleC force-pushed fix/issue/43/superkaramba-segv from 3ac0a96fbb to da9cd0c056 1 year ago
Poster
Owner

Latest commit addresses the points above. Still to look into the cmake target, will do that later in the afternoon

Latest commit addresses the points above. Still to look into the cmake target, will do that later in the afternoon
Poster
Owner

Looking at the cmake code, it seems the example files are not even part of the building process. In debian they are simply included in a package thanks to the packaging rules.
I propose we merge this PR first, then we modify the cmake files to include installation of examples from cmake. At the same time we can add a target to build xcursor.so at build time. What do you think?

Looking at the cmake code, it seems the example files are not even part of the building process. In debian they are simply included in a package thanks to the packaging rules. I propose we merge this PR first, then we modify the cmake files to include installation of examples from cmake. At the same time we can add a target to build xcursor.so at build time. What do you think?
SlavekB commented 1 year ago
Owner

Looking at the cmake code, it seems the example files are not even part of the building process. In debian they are simply included in a package thanks to the packaging rules.
I propose we merge this PR first, then we modify the cmake files to include installation of examples from cmake. At the same time we can add a target to build xcursor.so at build time. What do you think?

Yes, definitely, creating a CMake rules I assumed that it could be solved as a separate part.

> Looking at the cmake code, it seems the example files are not even part of the building process. In debian they are simply included in a package thanks to the packaging rules. > I propose we merge this PR first, then we modify the cmake files to include installation of examples from cmake. At the same time we can add a target to build xcursor.so at build time. What do you think? Yes, definitely, creating a CMake rules I assumed that it could be solved as a separate part.
SlavekB approved these changes 1 year ago
SlavekB left a comment
Owner

It looks good and ready for merge.

It looks good and ready for merge.
MicheleC merged commit da9cd0c056 into master 1 year ago
MicheleC deleted branch fix/issue/43/superkaramba-segv 1 year ago

Reviewers

SlavekB approved these changes 1 year ago
The pull request has been merged as da9cd0c056.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Reference: TDE/tdeutils#44
Loading…
There is no content yet.