Support for PEP384 + separate user script dir #1954
Conversation
src/plugins/python/pythonplugin.cpp
Outdated
| @@ -18,13 +18,15 @@ | |||
| * this program. If not, see <http://www.gnu.org/licenses/>. | |||
| */ | |||
|
|
|||
| #define Py_LIMITED_API 0x03040000 | |||
There was a problem hiding this comment.
I think this explains why you didn't run into the errors I was running into. The whole plugin needs to compile with this define, not only this file! In particular, we need pythonbind.cpp to also take this define.
Rather than putting this here, I have added the following line to python.qbs (see fa939ef):
cpp.defines: base.concat(["Py_LIMITED_API=0x03040000"])There was a problem hiding this comment.
Ah, I added the define in the Makefile but guess I never did make clean to force it to recompile the bind sources as well.
Now it does give those same errors, will check if there's something that can be done about them.
There was a problem hiding this comment.
yep it does indeed look like pybindgen would require many changes to generate stable ABI compatible code .. should’ve perhaps read past the PEP384 Abstract and/or realised that just removing one PyRun was way too easy. :)
The biggest hurdle seems to be the new dynamic type objects. There is however a tool that can automate the conversion to some extent http://svn.python.org/projects/python/branches/pep-0384/Tools/scripts/abitype.py ..I’ll give that a go some day next week when there’s a free moment.
Indeed, the example files were never installed, though they are listed in I think I preferred the contents of |
|
Ok, having modified pythonbind.cpp to use dynamic type declarations (PyType_FromSpec) + few other changes, the plugin now compiles with -DPy_LIMITED_API, and fotf.py and mappy.py successfully load maps on mac + linux as they used to. Re: _preload, I was thinking maybe there could be a feature for batch converting bunch of levels from custom binary formats to tmx and with that users might want to get any errors to stderr, having the logging redirection in a script gives them an option to customize it as they want to. |
There was a problem hiding this comment.
@stt That's a promising change, thanks!
Obviously the file can't be regenerated anymore without losing the changes. I opened an issue with pybindgen project for adding stable API support, if no one picks it up in a day or two I'll look into developing that feature.
Thanks for opening gjcarneiro/pybindgen#13, let's see what the response is.
How hard is it to do the changes? I guess I should hold off on merging this until it is either mostly automated or working in PyBindGen.
I've provided some more comments inline.
| if (QFile::exists(mScriptDir)) | ||
| mFileSystemWatcher.addPath(mScriptDir); | ||
| connect(&mFileSystemWatcher, SIGNAL(directoryChanged(QString)), | ||
| &mReloadTimer, SLOT(start())); |
There was a problem hiding this comment.
This connection is now already made in the constructor, so it can be removed here.
| /** | ||
| * (Re)load modules in the script directory | ||
| */ | ||
| void PythonPlugin::reloadModules() | ||
| void PythonPlugin::reloadModules(QString path) |
There was a problem hiding this comment.
Please use const QString &path to avoid unnecessary reference counting.
| @@ -40,7 +41,8 @@ static void handleError() | |||
| } | |||
|
|
|||
| PythonPlugin::PythonPlugin() | |||
| : mScriptDir(QDir::homePath() + "/.tiled") | |||
| : mSysScriptDir(QCoreApplication::applicationDirPath() + "/scripts") | |||
There was a problem hiding this comment.
Probably would prefer to call this mAppScriptDir.
Unfortunately, initializing it like this will only work on Windows. Check out the LanguageManager constructor for how we could make it work on macOS and Linux (and of course, install rules will need to be written for this - see translations.qbs).
| @@ -10,7 +10,7 @@ | |||
|
|
|||
|
|
|||
| #if PY_VERSION_HEX >= 0x03000000 | |||
| #if PY_VERSION_HEX >= 0x03050000 | |||
| #if PY_VERSION_HEX >= 0x03050000 && 0 // <- TODO: this is not Py_LIMITED_API compatible? | |||
There was a problem hiding this comment.
PyAsyncMethods is not available in the limited API, so that check should probably be:
#if PY_VERSION_HEX >= 0x03050000 && !defined(Py_LIMITED_API)
bjorn
force-pushed
the
master
branch
3 times, most recently
from
384d5ac
to
2e9a0fb
Compare
Todo: assumes
scriptsdirectory is under QCoreApplication::applicationDirPath(), if this is ok packaging scripts would need to be updated.Actually for me doing
make install INSTALL_ROOT=/tmp/tiled-pkgdoesn't copy any of the examples etc directories to INSTALL_ROOT that dist/distribute.qbs seems to indicate should be copied in the package?