-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
fix: ctypes (libffi) is no longer required on linux #455
Conversation
0ef248a
to
bd4d1f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fill out the PR template?
Signed-off-by: Morgan Epp <[email protected]>
bd4d1f5
to
6504639
Compare
|
@@ -94,13 +93,16 @@ def _is_relative_to(path1: Union[Path, str], path2: Union[Path, str]) -> bool: | |||
|
|||
|
|||
def _is_windows_file_path_limit() -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Not from this PR, but this name appears to be logically reversed from the function it calls. A name like _windows_are_long_paths_enabled
, matching the Windows API call very closely, could be more clear.
What was the problem/requirement? (What/Why)
A recent submitter installer release for Linux operating systems had a deadline executable that when run with anything would have a stack trace that looks like:
Effectively, a new system library dependency was now required by the
deadline
executable since we only allowlist 3 libraries to bundle with the pyinstaller build. Thus, Linux distros that use the new version of libffi (libffi.so.8) would be unable to run the executable.This was inadvertently introduced in a recent innocuous change when a ctypes import was added to the top of a file instead of being behind an
is_windows
check.What was the solution? (How)
There's some forward looking items we can do to build on each operating system we support and choose to install the right executable for the distro. In addition to automation around testing the installer automatically on those various platforms
However, this quick fix is to just remove the newly added libffi dependency for Linux. We do this everywhere else we use ctypes, so we're just pattern-matching here.
What is the impact of this change?
The submitter installer builds a
deadline
executable that works on newer linux distros.How was this change tested?
See DEVELOPMENT.md for information on running tests.
download
orasset_sync
modules? If so, then it is highly recommendedthat you ensure that the docker-based unit tests pass.
Was this change documented?
N/A
Is this a breaking change?
Nope
Does this change impact security?
Nope
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.