TDE
/
sip4-tqt
6
0
Fork 0
Code Issues 1 Pull Requests Releases Wiki Activity
Labels Milestones

Improvements for Python3 Support #7

Merged
SlavekB merged 2 commits from deleted-message into master 2 years ago
Conversation 7 Commits 2 Files Changed 3
aneejit1 commented 2 years ago
Collaborator

As part of the implementation of Python 3 support, the following improvements/fixes have been made:

  • python-tqt is reporting "underlying C/C++ object has been deleted". This is rather unhelpful, so a little code has been lifted from sip 4.19.23 to make it a little clearer about what is going wrong.
  • Generated modules have been failing to load under pre-Python 3.9 versions because the initialisation function has not been publicly exported (i.e. it appears as "LOCAL" rather than "GLOBAL" in the generated library). Code has been added to the generated source to ensure that the function is not hidden so the resultant module can actually be loaded successfully.

Signed-off-by: aneejit1 aneejit1@gmail.com

As part of the implementation of Python 3 support, the following improvements/fixes have been made: - python-tqt is reporting "underlying C/C++ object has been deleted". This is rather unhelpful, so a little code has been lifted from sip 4.19.23 to make it a little clearer about what is going wrong. - Generated modules have been failing to load under pre-Python 3.9 versions because the initialisation function has not been publicly exported (i.e. it appears as "LOCAL" rather than "GLOBAL" in the generated library). Code has been added to the generated source to ensure that the function is not hidden so the resultant module can actually be loaded successfully. Signed-off-by: aneejit1 <aneejit1@gmail.com>
aneejit1 added 1 commit 2 years ago
8ad3cac8e3 Improve the "underlying C/C++ object has been deleted" message
MicheleC commented 2 years ago
Owner

Could you amend the commit message? There is a duplicated consecutive "from" in it.

Could you amend the commit message? There is a duplicated consecutive "from" in it.
aneejit1 force-pushed deleted-message from 8ad3cac8e3 to 76444639b2 2 years ago
aneejit1 commented 2 years ago
Poster
Collaborator

'Tis done.

'Tis done.
aneejit1 added 1 commit 2 years ago
b275737d3c Ensure module initialisation is exported globally
aneejit1 force-pushed deleted-message from b275737d3c to 1d4d9cfcbe 2 years ago
aneejit1 changed title from Improve the "underlying C/C++ object has been deleted" message to Improvements for Python3 Support 2 years ago
aneejit1 commented 2 years ago
Poster
Collaborator

Things got a bit complicated after I fixed the "non-trivial designated initializers" error in python-tqt. The next thing that happened is that I started getting "initqt" or "PyInit_qt" for Python 2 and 3 respectively which is down to them having hidden visibility from the compiler option "-fvisibility=hidden" having been specified.

I've tried several things to get around this, but they mostly either didn't work or were rather messy. The aim was to add __attribute__ ((visibility("default"))) to the function definition. This works for Python 2, but not Python 3. Turns out that Python 3.0 to 3.8 define the function as extern "C" in the #define for PyMODINIT_FUNC (3.9 onwards includes the attribute). If you add the attribute between PyMODINIT_FUNC and SIP_MODULE_ENTRY, there's a warning and it's ignored. I did toy with defining the function without PyMODINIT_FUNC, or doing a temporary #define PyObject __attribute__ ((visibility("default"))) PyObject but neither option seemed to be a good idea.

The only simple way I found around this is to add a #pragma GCC visibility push(default) and a corresponding #pragma GCC visibility pop around the function which is done for gcc >= v4 or clang. I did add an end function to do the pop after the function's closing brace but removed it again as the compiler seems to be fine with placing the "pop" straight after the opening brace for the function. Even though this is redundant for Python 3.9 and above, I've not done anything to exclude it; it's more conditions to stop doing something that'll cause no harm.

python-tqt is now importing the generated modules without problems.

Things got a bit complicated after I fixed the "non-trivial designated initializers" error in python-tqt. The next thing that happened is that I started getting "initqt" or "PyInit_qt" for Python 2 and 3 respectively which is down to them having hidden visibility from the compiler option "-fvisibility=hidden" having been specified. I've tried several things to get around this, but they mostly either didn't work or were rather messy. The aim was to add ```__attribute__ ((visibility("default")))``` to the function definition. This works for Python 2, but not Python 3. Turns out that Python 3.0 to 3.8 define the function as ```extern "C"``` in the #define for ```PyMODINIT_FUNC``` (3.9 onwards includes the attribute). If you add the attribute between PyMODINIT_FUNC and SIP_MODULE_ENTRY, there's a warning and it's ignored. I did toy with defining the function without PyMODINIT_FUNC, or doing a temporary ```#define PyObject __attribute__ ((visibility("default"))) PyObject``` but neither option seemed to be a good idea. The only simple way I found around this is to add a ```#pragma GCC visibility push(default)``` and a corresponding ```#pragma GCC visibility pop``` around the function which is done for gcc >= v4 or clang. I did add an end function to do the pop after the function's closing brace but removed it again as the compiler seems to be fine with placing the "pop" straight after the opening brace for the function. Even though this is redundant for Python 3.9 and above, I've not done anything to exclude it; it's more conditions to stop doing something that'll cause no harm. python-tqt is now importing the generated modules without problems.
SlavekB reviewed 2 years ago
sipgen/gencode.c Outdated
"#endif\n"
"{\n"
"#if (defined(__GNUC__) && __GNUC__ >= 4) || defined(__clang__)\n"
"#pragma GCC visibility pop\n"
SlavekB commented 2 years ago
Owner

Could it be easier to define in sip-tqt.h equivalent of macro KDE_EXPORT and then use such macro instead of wrapping using #pragma?

Could it be easier to define in `sip-tqt.h` equivalent of macro `KDE_EXPORT` and then use such macro instead of wrapping using `#pragma`?
aneejit1 commented 2 years ago
Poster
Collaborator

I've been through that loop already and it doesn't work in this context. I initially created a SIP_MODULE_EXPORT macro and re-wrote the function definition as:

PyMODINIT_FUNC SIP_MODULE_EXPORT SIP_MODULE_ENTRY()

The problem is Python defines PyMODINIT_FUNC as extern "C" PyObject* so that the statement resolves itself to

extern "C" PyObject* __attribute__ ((visibility("default"))) PyInit_Qt()

after pre-processing which is not valid. The compiler issues a warning because of the extern "C" ("warning: 'visibility' attribute ignored on non-class types") then ignores the attribute causing the initialisation function to be hidden. The resultant syntax would have to be

extern "C" __attribute__ ((visibility("default"))) PyObject* PyInit_Qt()

for this to work, but that would mean not using PyMODINIT_FUNC at all. Since there are a number of possibilities for what PyMODINIT_FUNC could be set to in the Python headers, I figured it was safest not to try and override what it was doing.

The only other way I thought to do this was:

#define PyObject __attribute__ ((visibility("default"))) PyObject
PyMODINIT_FUNC SIP_MODULE_ENTRY()
#undef PyObject

which is a particularly ugly hack of a solution.

While #pragma isn't the nicest way to do this, in this particular situation it worked out to be the most reliable way to get the desired result.

I've been through that loop already and it doesn't work in this context. I initially created a ```SIP_MODULE_EXPORT``` macro and re-wrote the function definition as: ``` PyMODINIT_FUNC SIP_MODULE_EXPORT SIP_MODULE_ENTRY() ``` The problem is Python defines ```PyMODINIT_FUNC``` as ```extern "C" PyObject*``` so that the statement resolves itself to ``` extern "C" PyObject* __attribute__ ((visibility("default"))) PyInit_Qt() ``` after pre-processing which is not valid. The compiler issues a warning because of the ```extern "C"``` ("warning: 'visibility' attribute ignored on non-class types") then ignores the attribute causing the initialisation function to be hidden. The resultant syntax would have to be ``` extern "C" __attribute__ ((visibility("default"))) PyObject* PyInit_Qt() ``` for this to work, but that would mean not using PyMODINIT_FUNC at all. Since there are a number of possibilities for what PyMODINIT_FUNC could be set to in the Python headers, I figured it was safest not to try and override what it was doing. The only other way I thought to do this was: ``` #define PyObject __attribute__ ((visibility("default"))) PyObject PyMODINIT_FUNC SIP_MODULE_ENTRY() #undef PyObject ``` which is a particularly ugly hack of a solution. While #pragma isn't the nicest way to do this, in this particular situation it worked out to be the most reliable way to get the desired result.
SlavekB marked this conversation as resolved
SlavekB commented 2 years ago
Owner

Thank you, I understand, using #pragma seems to be the easiest viable solution.

Thank you, I understand, using `#pragma` seems to be the easiest viable solution.
SlavekB commented 2 years ago
Owner

Is it a good time to merge it? Or do you work on some other patches on this topic?

Is it a good time to merge it? Or do you work on some other patches on this topic?
aneejit1 commented 2 years ago
Poster
Collaborator

I've not got anything else to do with sip at the moment, so merge away!

I've not got anything else to do with sip at the moment, so merge away!
MicheleC reviewed 2 years ago
siplib/sip-tqt.h Outdated
#define SIP_SHARE_MAP 0x0040 /* If the map slot might be occupied. */
#define SIP_CPP_HAS_REF 0x0080 /* If C/C++ has a reference. */
#define SIP_POSSIBLE_PROXY 0x0100 /* If there might be a proxy slot. */
#define SIP_CREATED 0x1000 /* If the C/C++ object has been created. */
MicheleC commented 2 years ago
Owner

Shouldn't this be 0x0200 rather than 0x1000?

Shouldn't this be 0x0200 rather than 0x1000?
aneejit1 commented 2 years ago
Poster
Collaborator

I've copied from sip 4.19.23 which has several other flags defined that I'm not using here. Just in case more needs to be imported somewhere down the line, I figured it best to retain consistency between the two rather than have different values.

I've copied from sip 4.19.23 which has several other flags defined that I'm not using here. Just in case more needs to be imported somewhere down the line, I figured it best to retain consistency between the two rather than have different values.
SlavekB commented 2 years ago
Owner

Yes, retaining consistency in case we backport some other ideas from upstream SIP is a good idea.

Yes, retaining consistency in case we backport some other ideas from upstream SIP is a good idea.
MicheleC commented 2 years ago
Owner

Ok, makes sense. I had missed the point you had backported code from upstream. In that case we need to clearly state that in the commit message, together with link to source code and reference to the fact that the code is provided under GPL 2 or higher.

Ok, makes sense. I had missed the point you had backported code from upstream. In that case we need to clearly state that in the commit message, together with link to source code and reference to the fact that the code is provided under GPL 2 or higher.
aneejit1 commented 2 years ago
Poster
Collaborator

Is there a specific wording or format you need? Perhaps an example?

Is there a specific wording or format you need? Perhaps an example?
aneejit1 commented 2 years ago
Poster
Collaborator

OK, 'tis done.

OK, 'tis done.
MicheleC commented 2 years ago
Owner

Looks good, thanks for amending. Licensing and author copyright is an important thing, so we always have to be careful and transparent :-)

Looks good, thanks for amending. Licensing and author copyright is an important thing, so we always have to be careful and transparent :-)
MicheleC marked this conversation as resolved
MicheleC commented 2 years ago
Owner

No specific wording, but you can look at the message here or here

No specific wording, but you can look at the message [here](https://mirror.git.trinitydesktop.org/gitea/MicheleC/tdeio-appinfo/commit/5a34fb833421c38e91408864abc94af79f3bc152) or [here](https://mirror.git.trinitydesktop.org/gitea/TDE/polkit-agent-tde/commit/25813d39a534b4e2e2bbc785a18589e1f1d50317)
aneejit1 force-pushed deleted-message from 1d4d9cfcbe to b9448655a2 2 years ago
SlavekB approved these changes 2 years ago
SlavekB left a comment
Owner

Thank you for your great cooperation.
Everything looks ready for merge.

Thank you for your great cooperation. Everything looks ready for merge.
SlavekB merged commit b9448655a2 into master 2 years ago
SlavekB deleted branch deleted-message 2 years ago
SlavekB added this to the R14.0.13 release milestone 2 years ago

Reviewers

SlavekB approved these changes 2 years ago
The pull request has been merged as b9448655a2.
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.0.13 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/sip4-tqt#7
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'deleted-message'

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

No
Yes