Fix PyInstaller hook

[rokm]

The PyInstaller hook was added in #162, but currently its entry-points are defunct in real-world installations (PyPI wheels, non-editable installs) because freetype.__pyinstaller is not installed as part of the freetype-py.

This is masked on the CI because editable install is used, and also because freetype in the frozen application will happily use the system-available copy of the freetype shared library if the bundled one is not available. So a basic import test is insufficient, especially on a linux-based CI runner, where the system-available copy of freetype shared library is guaranteed to be available.

[HinTak]

A few comments:

• Not sure about renaming and moving files
• the logical way of making your pull convincing is to add macos x and windows testing.
• editable install is available on every system: you can do "pip install --user", and in fact, run freetype-py without installing, by setting PYTHONPATH . I do that all the time, because my local freetype-py repo have some extra bits not in the system one.

So I am not convinced about how you are doing it (moving files without explaining, and not adding mac os x CI at least, if not Windows also), and why you are doing it (editable install is always available).

[Korijn]

This PR looks great to me, you fixed it the proper way. I can't believe I missed the setup.py changes in #162, sorry about that.

Regarding @HinTak's feedback:

• Not sure about renaming and moving files

It's okay to reorganize the unit tests for the pyinstaller hook into their own folder, I think. It's a minor change with no impact beyond the pyinstaller hook.

• the logical way of making your pull convincing is to add macos x and windows testing.

We could consider adding two more CI jobs to test the pyinstaller hook on macos and windows, but I doubt it will make any difference. I don't think it's worth the added build complexity and time for such a small feature.

In fact, the linux CI job is the most challenging one since it has a system freetype available and a bundled freetype. So it's a good environment to test that our hook prefers the bundled build.

All in all, the ubuntu CI job for the pyinstaller hook is sufficient IMHO.

• editable install is available on every system: you can do "pip install --user", and in fact, run freetype-py without installing, by setting PYTHONPATH . I do that all the time, because my local freetype-py repo have some extra bits not in the system one.

We're not breaking or changing anything about the ability to do an editable install. The point of this PR is to specifically address the use case where users have installed freetype-py from a wheel on pypi to use in their project, and create a distributable of their project using pyinstaller.

If you are concerned that we are not supporting the scenario where a user runs pyinstaller on a project with an editable freetype-py install, that's also supported. However, this PR is only intended to address a bug in the current packaging setup. It's not adding a new feature or expanding the set of use cases that are supported.

The -e flag in the CI job for the pyinstaller test was removed in order to intentionally provoke the bug (as mentioned in the description: "This is masked on the CI because editable install is used"), so that our test suite can guard against regressions in the future. I think that's a great improvement to the CI script.

So I am not convinced about how you are doing it (moving files without explaining, and not adding mac os x CI at least, if not Windows also), and why you are doing it (editable install is always available).

I am convinced of the correctness of this PR and would vote to merge it. We can always open another PR to improve it further. For now it addresses the bug correctly.

[HinTak]

Besides the moving files, there are a number of unnecessary(?) code changes bundled too (the import statement changes). The problem with this pull in its current form is that it is hard to tell which part is necessary and which part is a matter of opinion/style. Stylistic changes are a matter of opinion.

[Korijn]

Besides the moving files, there are a number of unnecessary(?) code changes bundled too (the import statement changes). The problem with this pull in its current form is that it is hard to tell which part is necessary and which part is a matter of opinion/style. Stylistic changes are a matter of opinion.

The only changes of that nature I can see are in freetype/__pyinstaller/__init__.py, and the moving of the pyinstaller hook test suite into its own tests subfolder.

The import had to be changed because we're now using os.path.join besides just os.path.dirname. So only the change on line 4 is a consequence of the stylistic preference of import.

Beyond that it's very easy to tell from the diff that the tests files were moved without other stylistic changes, beyond what was necessary to improve the test as explained in the PR description.

I think it's a little unreasonable to block this PR on that.

To summarize the changes in this PR:

• The changes in setup.py are necessary for the hook to be functional when freetype is installed as a package (usually a wheel from pypi). That's actually the core of this PR, and what fixes the bug
• The changes to the CI script and pyinstaller tests suite are there to ensure CI prevents this bug from reappearing
• There is the stylistic change of the os.path import
• The pyinstaller hook test suite was moved into its own subfolder

I hope this makes it easier to understand the PR and addresses your concerns.

[HinTak]

Nobody is "blocking" a pull. Pulls are accepted IMO when they are convincing as address a bug or add a feature. The latter is easier - if a pull has no effect on existing usage, it is usually accepted (hence the initial work, #162 got in quite quickly without much question). I.e. a user uses the new functionality knowing that it is new and possibly buggy. If a pull is supposed to address a bug, then it should just do only that, and not introduce either stylistic changes or rearrangements more than necessary. So it is harder to be accepted, or even be looked at.

The worst - a change buried in an rearrangement. Personally I don't want to spend time checking whether such an event has happened, so if the pull looks too much, I don't spend time looking at it. That's purely my own opinion.

[Korijn]

Ok. So then we can merge this?

[HinTak]

As I already wrote, I don't want to spend time looking at "more than necessary" code. Other's opinion might differ.

[Korijn]

I have time aplenty to look at such code but no rights to merge.

I have to say I am a bit puzzled as to why you decided to cc me. I invested a lot of time to help you review the PR and you're essentially just disregarding all of it. I would appreciate not being cc'd again if it doesn't make a difference for the end result anyway. I will unsubscribe now and leave it to others.

[HinTak]

Also not sure what's the purpose of this code - freetype-py (and freetype) are available on all major Linux platforms, so it seems a bit strange to do bundle for Linux (versus other platforms), and also only test on Linux. Anyway, I'll leave this to others who want to look at it.

[Korijn]

@rougier would you mind merging this? The pyinstaller hook is missing from the packages kwarg in setup.py, meaning that the hook is not shipped as a part of the wheels on pypi, making it unavailable to freetype-py users. I somehow overlooked this in #162, for which I apologize. This PR is a great, concise fix of the bug in setup.py, and includes an improvement to the hook test.

Also not sure what's the purpose of this code - freetype-py (and freetype) are available on all major Linux platforms, so it seems a bit strange to do bundle for Linux (versus other platforms), and also only test on Linux.

The pyinstaller hook is not used to create freetype distributables. I'll try to explain what pyinstaller is, how it is used, and why it's valuable and worthwhile to include a pyinstaller hook in freetype-py. Hopefully this will also help you understand why it's not necessary to execute the hook test on other platforms.

• Pyinstaller is a tool that end-users apply when they want to create a portable distributable version of their python applications, including all of its dependencies (which can include freetype-py). Pyinstaller is able to analyze the source code statically to identify all code that needs to be included in the distributable.
• Pyinstaller cannot detect dynamically included code, such as dynamic imports and dynamically loaded binaries. The binary included in the freetype-py wheels on pypi is one such file that is not automatically detected by pyinstaller's static code analysis. Yet, the freetype binary must be included when pyinstaller creates a distributable for a given python application that depends on freetype-py.
• This is where the hook comes in. Pyinstaller queries the python environment for registered pyinstaller40 entrypoints to find hooks. You can see this entrypoint is in setup.py in the master branch already.
• Pyinstaller then executes the hook (hook-freetype.py) which we have configured to include the freetype binary. This is how we ensure pyinstaller will include the freetype binary in the distributable.

The bug that this PR is fixing, is that the hook is not shipped as part of the wheels. So what happens with the current release of freetype-py, is that pyinstaller will query the entrypoint, try to import the hook, and then fail.

So, in summary, by shipping the pyinstaller hook in freetype-py, end users will be able to create distributables of python applications that depend on freetype-py.

[rokm]

An alternative solution (if @Korijn is fine with it) would be to remove the hook code and related entry-point from the freetype-py (essentially reverting #162), and I add the hook to the pyinstaller-hooks-contrib repository instead (and there we can seamlessly test the hook against the freetype-py PyPI wheels on all OSes).

The removal here is not really necessary per se (if freetype-py fixes their hook, it would take precedence over the copy provided by PyInstaller / pyinstaller-hooks-contrib), but on the other hand, a) the incomplete entry-point results in a warning (see https://github.com/rougier/freetype-py/issues/181) during PyInstaller runs, b) carrying code that serves no real purpose is kind of pointless.

[Korijn]

I'm okay with any path that results in functional pyinstaller support for freetype-py.

So for @rougier that's either merging this PR, or removing the hook entirely in a new PR and maintaining the hook externally.

[rokm]

Or merging https://github.com/rougier/freetype-py/pull/179, if its is preferable to leave tests as they are, and just fix the entry-point.

[Korijn]

I appreciate the transparency @rokm but for @rougier as a TL;DR I would recommend to merge this PR. :)

[HinTak]

Freetype-py is available on all Linux platforms already packaged, so it doesn't make sense to re-package something already packaged. If somebody wants to bundle freetype-py with something else, then such (re-)packaging-related code naturally belongs to that 'something else', rather than freetype-py. And if somebody wants to re-package/bundle freetype-py, then they should work from source rather than from pre-packaged package.

There is a need to bundle freeype-py with feeetype and a python interpreter on non-linux. Hence, I would say, remove this code, or make CI works with non-linux.

That said, the moving/relocation in this pull is still questionable.

[anthrotype]

it doesn't make sense to re-package something already packaged.

that's just your opinion. Others may prefer their python app to be self-contained, not have external non-python requirements and control exactly which version of freetype they use.

Anyway so much digital ink wasted.. this PR does no harm and actually fixes a bug, so I'm going to merge it. Thanks @rokm and @Korijn

[HinTak]

@anthrotype There is a point where merging random additional code/dependency needs to stop, for long term maintenance. If anything that a user might needs as a singular convenience should be merged, I might propose including the whole of skia-python as a dependency to provide OT-SVG color font support. That's a 30MB blob on most platforms.

[HinTak]

The fact that non-needed recent addition causes problems may be an indication that it should be removed.

[HinTak]

So I am suggesting that if there is another issue filed with this code, it should be removed.