Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add hidden import to PyInstaller build #2466

Merged
merged 2 commits into from Sep 1, 2021
Merged

Add hidden import to PyInstaller build #2466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef50ecc
Add hidden import to PyInstaller build
jalaziz Aug 31, 2021
07f6998
Only include the platformdirs for the relevant os
jalaziz Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/upload_binary.yml
View file
Open in desktop
Expand Up @@ -40,8 +40,11 @@ jobs:
python -m pip install pyinstaller
- name: Build binary
run: |
python -m PyInstaller -F --name ${{ matrix.asset_name }} --add-data 'src/blib2to3${{ matrix.pathsep }}blib2to3' src/black/__main__.py
run: >
python -m PyInstaller -F --name ${{ matrix.asset_name }} --add-data
'src/blib2to3${{ matrix.pathsep }}blib2to3' --hidden-import platformdirs.unix
--hidden-import platformdirs.macos --hidden-import platformdirs.windows
src/black/__main__.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jalaziz jalaziz Aug 31, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@ichard26 ichard26 Sep 1, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

- name: Upload binary as release asset
uses: actions/upload-release-asset@v1
Expand Down
6 changes: 6 additions & 0 deletions CHANGES.md
View file
Open in desktop
@@ -1,5 +1,11 @@
# Change Log

## Unreleased

### Packaging

- Fix missing modules in self-contained binaries (#2466)

## 21.8b0

### _Black_
Expand Down