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 3 years ago
Conversation 7 Commits 2 Files Changed 3
aneejit1 commented 3 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 3 years ago
8ad3cac8e3 Improve the "underlying C/C++ object has been deleted" message 
python-tqt is reporting "underlying C/C++ object has been deleted". This
is rather unhelpful, so a little code has been lifted from from sip
4.19.23 to make it a little clearer about what is going wrong.

Signed-off-by: aneejit1 <aneejit1@gmail.com>
MicheleC commented 3 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 3 years ago
aneejit1 commented 3 years ago
Poster
Collaborator

'Tis done.

'Tis done.
aneejit1 added 1 commit 3 years ago
b275737d3c Ensure module initialisation is exported globally 
The generated module initialisation function fails to export because of
the "-fvisibility-hidden" resulting in an import failure on missing
symbol. Alter the visibility of the initialisation function to "default"
to ensure that it winds up as "global" in the module binary.

Signed-off-by: aneejit1 <aneejit1@gmail.com>
aneejit1 force-pushed deleted-message from b275737d3c to 1d4d9cfcbe 3 years ago
aneejit1 changed title from Improve the "underlying C/C++ object has been deleted" message to Improvements for Python3 Support 3 years ago
aneejit1 commented 3 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 3 years ago
sipgen/gencode.c Outdated
@ -2331,2 +2334,4 @@
"#endif\n"
"{\n"
"#if (defined(__GNUC__) && __GNUC__ >= 4) || defined(__clang__)\n"
"#pragma GCC visibility pop\n"
SlavekB commented 3 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 3 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 3 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 3 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 3 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 3 years ago
siplib/sip-tqt.h Outdated
@ -1432,6 +1432,7 @@ typedef struct _sipTQtAPI {
#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 3 years ago
Owner

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

Shouldn't this be 0x0200 rather than 0x1000?
aneejit1 commented 3 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 3 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 3 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 3 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 3 years ago
Poster
Collaborator

OK, 'tis done.

OK, 'tis done.
MicheleC commented 3 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 3 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 3 years ago
SlavekB approved these changes 3 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 3 years ago
SlavekB deleted branch deleted-message 3 years ago
SlavekB added this to the R14.0.13 release milestone 3 years ago

Reviewers

SlavekB approved these changes 3 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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

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