silnrsi / graide

GRaphite Integrate Development Environment
GNU Lesser General Public License v2.1
13 stars 2 forks source link

QtPy, Python 3, upstream dependencies #3

Closed bgermann closed 5 years ago

bgermann commented 6 years ago

This makes graide compatible with QtPy and Python 3. QtPy forwards the Qt5 API calls to one of four backends, including PySide and the Qt5-based PySide2. Python 2 is currently phased out on many distributions because its end of life is scheduled on 1.1.2020.

The vendored, old freetype-py dependency is removed and replaced by a dependency on the upstream module that is now available with binaries on PyPI. Additionally, the graide.graphite module is replaced with the upstream graphite2 module. This feature depends on https://github.com/silnrsi/graphite being published on PyPI.

Furthermore, a feature to compile grcompiler on creating a wheel is introduced.

mhosken commented 6 years ago

May I ask what is to be gained by switching to qtpy over pyside?

bgermann commented 6 years ago

On the PySide source repository there is a big warning "ATTENTION: This project is deprecated, please refer to PySide2." The main reason is that PySide is a Qt4 binding and Qt4 is end of life and currently phased out by many Linux distributions. So why not use PySide2? I chose QtPy because you can use PySide or PySide2 as a backend. So people that absolutely want to stick to Qt4 can use PySide. And it was easier to port as I could just change the QT_API environment variable to use either PySide or PySide2. However QtPy has exactly the same API as PySide2 so the only difference are the import statements and those can be changed quite easily.

bgermann commented 6 years ago

I think this is ready to go now. It would make sense to stop exporting installation files with PyInstaller and to publish graide on PyPI instead. Then a user would be able to install graide via pip install graphite-graide and pip install PySide(2) on each supported platform. Renaming the package to graphite2-graide or just graide would make sense because there is an unrelated graphite package on PyPI already.

mhosken commented 6 years ago

Initial read through. Things look good. A couple of questions/issues:

. Does this still run fine on python2.7. I noticed the loss of a couple of unicode() calls and switching isinstance(x, basestring) to isinstance(x, str) may cause problems. . Losing the bundling of dependencies for Windows users could be a bit of a pain. I suppose pip solves all that now?

bgermann commented 6 years ago

I made sure this still runs on Python 2.7. That is what future's builtins.{chr,str} are for.

Pip solves all dependencies, even on Windows. In fact, all popular Python projects are moving from bundling the Python interpreter and dependencies incl. binaries in a setup.exe to using wheels. That has the advantage of the user being in control of the Python versions that they have installed with their potential security issues.

devosb commented 6 years ago

@bgermann I admire the carefulness to not break Python 2.7 support. @mhosken Why do we need Python 2.7 compatibility?

mhosken commented 6 years ago

OK I've started a more in depth review of this PR.

  1. GRCOMPILER_BUNDLE_VERSION=master python setup.py build doesn't build grcompiler. It only builds for bdist_wheel (which is an undocumented setup.py according to my setup.py --help-commands).

  2. Please don't make people have to install a toolchain for grcompiler just to use graide from source. If you want to build a fresh grcompiler during bdist, that's fine, but not just to let people run the thing. The same applies to the other .dlls. Could we make it that if you do setup.py without setting GRCOMPILER_BUNDLE_VERSION, it uses the bundled version otherwise it builds it?

  3. ttfrename should not be bundled as a separate app. It's old test code and isn't a releasable app. It is also wrongly named since it clashes with ttfrename which ships with the fontutils package (yeh it's written in perl, but it still is a valid tool on people's systems). Best just ignore that it is there and I should probably remove it.

Please bare in mind that a primary use case is not pip based but Ubuntu package based. So any dependencies must already be available as debian packages and not just in pip. I'm OK with the bdist process for pip shedding supporting libraries in favour of cross pip dependencies, but I would still like to see the supporting libraries left in the repo, at least for now.

Having said all this. I do like the idea of people installing using pip rather than an installer .exe. I'm fine with pyinstaller going away.

I'm intrigued why you got drawn into working on graide. It's a bit of a corner product.

bgermann commented 6 years ago

The three changes that you suggested are in the PR now. By default, the binaries are not bundled (as this is what you want for most setup() calls. For bdist* one can set GRCOMPILER_BUNDLE to an arbitrary value to get the prebuilt version).

The missing Debian packages are python-freetype and python-graphite2. I am building a python-freetype Debian package (not published yet) and extended PR https://github.com/silnrsi/graphite/pull/37 to include Debian python packages.

bgermann commented 5 years ago

Thank you for merging this. Maybe you want to take the last two commits as well.