[sipify] make all protected methods slots

[3nids]

fixes #45331

[3nids]

The spell test can discarded for now, I'm not in a mood of hacking sipify to support typos in headers.

[nyalldawson]

Fix looks acceptable to me. But I'm curious what the actual cause of this change was... did older sip versions silently treat public as private?

[nirvn]

What's the status on this? I'm missing my python algorithms 😉

[3nids]

This can be merged, only failing test is the spell check which I don't want to handle (it's gonna file once when this is merged, and at every change in /core/layout/qgslayoutexporter.h until the typo is fixed in QGIS 4)

@nyalldawson ok for you?

[nirvn]

@3nids , is there a reason why we double spell check here (i.e. source .h[eader] and the derived .sip.in files)? With source .h[eader] we have the #spellok tag to make the spellcheck happy. Could we just skip .sip.in files altogether since they are nowadays always generated from the headers?

[nirvn]

@3nids , thanks for this fix!

[nirvn]

@3nids , you'll need to append a ':%' suffix to the spelling.dat's seperate line (https://github.com/qgis/QGIS/blob/master/scripts/spell_check/spelling.dat#L6422) .

While at it, you could improve this regexp in check_spelling.sh (https://github.com/qgis/QGIS/blob/master/scripts/spell_check/check_spelling.sh#L254) from

              elif [[ "$FILE" =~ \.(h|cpp|sip)$ ]] && [[ "$ERRORLINE" =~ ^\s*(\/*\|\/\/) ]]; then

to

              elif [[ "$FILE" =~ \.(h|cpp|sip(\.in)?)$ ]] && [[ "$ERRORLINE" =~ ^\s*(\/*\|\/\/) ]]; then

to catch the .sip.in files (although I think that's a pointless change since .sip.in seems to strip the #spellok tag.

[nirvn]

@3nids , @nyalldawson , I've rebased this against master to fix the conflict. Let's merge this as soon as it turns green to avoid further conflicts.

[nirvn]

Hm, so just built this PR, and I still can't run python algorithms, error:

RuntimeError: no access to protected functions or signals for objects not created from Python 
Traceback (most recent call last):
  File "/home/webmaster/apps/share/qgis/python/plugins/processing/gui/ProcessingToolbox.py", line 253, in executeAlgorithm
    in_place_input_parameter_name = alg.inputParameterName()
RuntimeError: no access to protected functions or signals for objects not created from Python

I'll make clean and rebuild from scratch, but that doesn't look good :/

[nirvn]

OK, so, if I remove 'protected slots:' in the qgsprocessingalgorithm.sip.in (i.e., making those functions all public:), I can run algorithms again.

So, going protected -> protected slots isn't enough, it seems we would need to go from protected -> public. Is that acceptable? If not, I fear we'll have to revert the original sip changes which caused the regression for now.

[nirvn]

Unrelated test failure (3d). All is green, let's merge?

[nyalldawson]

yep

[jef-n]

This breaks the MSVC build, which doesn't define -Dprotected=public because that triggers:

xkeycheck.h(332): fatal error C1189: #error: The C++ Standard Library forbids macroizing the keyword "protected". Enable warning C4005 to find the forbidden define.

Above can be avoided by defining -D_ALLOW_KEYWORD_MACROS -Dprotected=public, but that in turn causes a truck load of linker errors.

Reverting fee62e4 and dff05dd and rerunning sipify_all.sh (effectivly reverting 3fb0f66) apparently fixes the build in that aspect... (a new linker error in testqgsmesheditor.cpp just popped up)

[nyalldawson]

Ping @manisandro are you able to investigate this with a matter of urgency? If we can't resolve before Friday we'll need to revert all the sip cmake changes prior to release...

[jef-n]

um, "all" wouldn't be good either - osgeo4w now has SIP 6…
I even backported the changes to 3.16 and 3.20 (as patch in the recipe) - master is just the last stop…

The MSVC build just completed (https://cdash.orfeo-toolbox.org/buildSummary.php?buildid=87711). Trying it now on Linux (looks good so far).

In all it takes much longer than I had expected (since sunday night so far)…

[jef-n]

BTW do we want python code to access protected members, although it's not in method of a derived class? That's what #45331 is about, isn't it? Or am I missing something?

[nyalldawson]

@jef-n

BTW do we want python code to access protected members, although it's not in method of a derived class?

Ideally no, but unfortunately they're effectively part of the stable api guarantee now, and there IS client code which access them out there (e.g. processing)

[jef-n]

Arguable. IMHO it's still a bug in the client code if it uses a protected member. But if we protected something that actually needs to be public to be useful, it should not be protected to start with. OTOH the pyqgis documentation doesn't tell whether something is protected or not unless it explicitly mentioned in the description (probably because that concept doesn't exist in python).

[nyalldawson]

@jef-n

Arguable.

Only from a legalistic point of view. As you've pointed out, Python has no concept of these and we've been happily exposing the methods in the pyqgis docs, in python dir(object) results, in IDE autocomplete, etc for over a decade. It's not reasonable to expect that Python devs have had to check the c++ code/docs to determine if every method exposed in PyQGIS is supposed to be used or not.

We CAN'T release with this regression. The consequences of knowingly breaking stable API in such a wholesale manner (especially mid-way through the 3.16 LTR release) are mind numbing... 😱

[jef-n]

Hm, can't reproduce #45331 with the MSVC build. multiparttosingleparts works for me from the toolbox. And this in the python console:

QgsApplication.processingRegistry().algorithmById("native:multiparttosingleparts").inputParameterName() 'INPUT'

as in 3.20 and 3.16.

I wonder why we need to turn everything public explicetely at all. As python doesn't have protected methods, SIP should need to make them public already to make them available for derived classes (even if it probably cannot restrict their use to derived classes only). I guess the protected=public hack just makes it more efficient.

[nyalldawson]

@jef-n
Which sip version is that with?

[jef-n]

@jef-n Which sip version is that with?

sipbuild.version.SIP_VERSION_STR
'6.1.1'

[nyalldawson]

So maybe the issue is only reproducible with sip v 5? (I'm stuck on 4.19, so can't test myself...)

[3nids]

if needed, we could make this depending on the SIP version.
(this needs to be handled in the .sip.in files and when we run cmake, the produced .sip files will depend on the sip version)

[manisandro]

I only tested with sip6 on my part and could reproduce the issue with sip6. Rather, if #45331 is not reproducible with the MSVC build, can the protected -> public replacement be made only on non-MSVC?

[jef-n]

So maybe the issue is only reproducible with sip v 5? (I'm stuck on 4.19, so can't test myself...)

Apparently not on MSVC at least. osgeo4w v2 has been using sip5 all along until the support was broken by the sip 6 updates. And on the 3.16 and 3.20 release builds I also get 'INPUT'. But I upgraded to SIP6 to get it building again - until this broke it again.

[jef-n]

sip6 with --no-public-is-protected also works on Linux:

diff --git a/cmake/SIPMacros.cmake b/cmake/SIPMacros.cmake
index 21328c9d83..4a9c400e6f 100644
--- a/cmake/SIPMacros.cmake
+++ b/cmake/SIPMacros.cmake
@@ -101,7 +101,7 @@ MACRO(GENERATE_SIP_PYTHON_MODULE_CODE MODULE_NAME MODULE_SIP SIP_FILES CPP_FILES
       ENDIF( ${CONCAT_NUM} LESS ${SIP_CONCAT_PARTS} )
     ENDFOREACH(CONCAT_NUM RANGE 0 ${SIP_CONCAT_PARTS} )
 
-    SET(SIPCMD ${SIP_BUILD_EXECUTABLE} --pep484-pyi --no-make --concatenate=${SIP_CONCAT_PARTS} --qmake=${QMAKE_EXECUTABLE} --include-dir=${CMAKE_CURRENT_BINARY_DIR} --include-dir=${PYQT5_SIP_DIR} ${SIP_BUILD_EXTRA_OPTIONS})
+    SET(SIPCMD ${SIP_BUILD_EXECUTABLE} --no-protected-is-public --pep484-pyi --no-make --concatenate=${SIP_CONCAT_PARTS} --qmake=${QMAKE_EXECUTABLE} --include-dir=${CMAKE_CURRENT_BINARY_DIR} --include-dir=${PYQT5_SIP_DIR} ${SIP_BUILD_EXTRA_OPTIONS})
 
     ADD_CUSTOM_COMMAND(
       OUTPUT ${_sip_output_files}

and then also produces 'INPUT'

[jef-n]

@nyalldawson so?

[nyalldawson]

@nirvn are you able to test this?

[nirvn]

@nyalldawson , @jef-n , building now.

[nirvn]

@nyalldawson , this commit (jef-n@7219b1f) works for me. Good job @jef-n .