Add hidden import to PyInstaller build

[jalaziz]

Description

Add new platformdirs dependencies as hidden imports when creating
PyInstaller-based binaries.

platformdirs imports the module for each platform dynamically, which
PyInstaller is unable to correctly detect for packing. By adding the
modules as hidden imports, we are telling PyInstaller to include the
modules in the packaged binary.

This issue seems to have been introduce when switching to platformdirs
in #2375.

fixes #2464

Checklist - did you ...

• Add a CHANGELOG entry if necessary?
• Add / update tests if necessary?
• Add new / update outdated documentation?

[felix-hilden]

Thanks for the PR! I'm not familiar with the system in place for building binaries, so I can't comment on the solution. But I think this could warrant a changelog entry, since it is clearly fixing a problem!

[jalaziz]

@felix-hilden added a changelog entry. Please let me know if it needs any adjustment.

[cooperlees]

Just asking a question to see if we should maybe inform (via an issue) platformdirs about this? This is a change of behavior from appdirs.

[cooperlees]

Just a question - Is there a fix we can do to platformdirs to not require this? I guess a follow on question is what is the difference with what platformdirs does vs. appdirs? Does it divide up each platforms into sub modules?

[jalaziz]

It looks like appdirs doesn't use dynamically loaded modules per platform, while platformdirs does.

The dynamically loaded modules is what throws PyInstaller off as it doesn't see a direct import statement and, understandably, can't predict what would be imported.

We could technically be smarter and import the correct module for each platform being built, but I went with the simpler fix for now. Should be an easy update though.

[ichard26]

The fact platformdirs breaks PyInstaller doesn't surprise me knowing how it works, but what does is that we have conditional imports in our code and yet they're not broken? The main culprit I have in mind is typing-extensions... it's not like the base install of Black has dependencies that then unconditionally depend on it. I guess sys.version_info guarded imports are treated correctly?

Or I could be misunderstanding all of this which is totally possible as it's 11 PM over here ^^

[jalaziz]

According to PyInstaller:

Hidden imports can occur when the code is using import(), importlib.import_module() or perhaps exec() or eval(). Hidden imports can also occur when an extension module uses the Python/C API to do an import. When this occurs, Analysis can detect nothing. There will be no warnings, only an ImportError at run-time.

That seems to suggest that conditional imports are fine, but dynamic imports aren't. I haven't looked into exactly how PyInstaller detects imports, but I suspect they some version of searching for import statements.

[ichard26]

Oh right, conditional import != hidden import. G'ah I should really get some sleep 😅 I was under the assumption that platformdirs did conditional imports as we do, but looking at the source, nope!

[jalaziz]

I went ahead and updated this PR with a small enhancement that only includes the platformdirs module for the platform being built against. I've briefly tested the linux and macos binaries. Test binaries can be found at https://github.com/jalaziz/black/releases/tag/21.8b1.

[jalaziz]

Just asking a question to see if we should maybe inform (via an issue) platformdirs about this? This is a change of behavior from appdirs.

Technically, the change in behavior should be fine. It just break things when packaging via PyInstaller. It could be argued that this is better as it allow a slightly smaller binary by excluding modules that aren't needed for a particular OS.

[ichard26]

I can test the Windows binary tomorrow if no one beats me to it. I doubt there's going to be an issue with it, but I'd rather play it safe. Thanks for the awesome work and for reporting the issue in the first place!

[ichard26]

Tested the generated binary on Windows and it works totally fine. Beautiful, thank you!

[ichard26]

Just asking a question to see if we should maybe inform (via an issue) platformdirs about this? This is a change of behavior from appdirs.

They seem to be aware of this issue: tox-dev/platformdirs#40 but given that the only maintainer response has been the addition of the "enhancement" and in particular "help wanted" labels in a ~week... it might take a while for that to be resolved. I'd rather prefer to unbreak this for our users first, and then get rid of it if those folks end up working around or fixing the issue.

---

Thank you so much for your contribution! This project is only possible by contributions like these 🖤 @jalaziz. Congrats on your commit to psf/black! If you have any feedback re. contributing, please tell us know via #2238.

[jalaziz]

Thank you for the quick turnaround! Always happy to contribute. Turns out I ran into this a couple weeks back and thought it was something that the binary build already took care of. Didn't realize the platformdirs change wasn't released yet at the time.

With regards to tox-dev/platformdirs#40, I was actually looking into the hook system that is being recommended there but thought that it would be a bit awkward for a project like black. Didn't cross my mind that platformdirs could host the hook file.

That being said, any chance of this fix being published soon given that the 21.8b0 binaries are broken?

[jalaziz]

@ichard26 Looks like PyInstaller has a contrib repo for hooks and support was added for platformdirs recently: https://github.com/pyinstaller/pyinstaller-hooks-contrib/blob/master/src/_pyinstaller_hooks_contrib/hooks/stdhooks/hook-platformdirs.py.

Once a new version of the contrib repo is released, we should be able to remove the manual imports (the PyInstaller docs suggest that the contrib package is installed automatically).