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

Tweaks for Windows portability #201

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Tweaks for Windows portability #201

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
8faed8b
Tweaks for Windows portability
jvolkman Feb 6, 2024
8ffe7ac
More fixes
jvolkman Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions delocate/delocating.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import logging
import os
import shutil
import stat
import warnings
from os.path import abspath, basename, dirname, exists, realpath, relpath
from os.path import join as pjoin
Expand All @@ -28,6 +29,7 @@

from .libsana import (
_allow_all,
filter_system_libs,
get_rp_stripper,
stripped_lib_dict,
tree_libs,
Expand Down Expand Up @@ -56,6 +58,12 @@ class DelocationError(Exception):
pass


def posix_relpath(path: str, start: str = None) -> str:
"""Return path relative to start using posix separators (/)."""
rel = relpath(path, start)
return Path(rel).as_posix()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the goal is cross platform portability then do not use str for path types in new functions.



def delocate_tree_libs(
lib_dict: Mapping[Text, Mapping[Text, Text]],
lib_path: Text,
Expand Down Expand Up @@ -157,7 +165,7 @@ def _analyze_tree_libs(
# @rpath, etc, at this point should never happen.
raise DelocationError("%s was expected to be resolved." % required)
r_ed_base = basename(required)
if relpath(required, rp_root_path).startswith(".."):
if Path(rp_root_path) not in Path(required).parents:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if Path(rp_root_path) not in Path(required).parents:
if not Path(required).is_relative_to(rp_root_path):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_relative_to is new in 3.9 and it looks like you're testing back to 3.7.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops. That's easy to miss when I'm not using an IDE.

Python 3.8 EOL is this year. Maybe I can justify dropping support for it? The path in path.parents syntax looks terrible even compared to relpath(...).startswith("..").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose one could say the syntax isn't great, but the docs do list root in path.parents as equivalent to is_relative_to.

# Not local, plan to copy
if r_ed_base in copied_basenames:
raise DelocationError(
Expand Down Expand Up @@ -202,6 +210,10 @@ def _copy_required_libs(
"Copying library %s to %s", old_path, relpath(new_path, root_path)
)
shutil.copy(old_path, new_path)
# Make copied file writeable if necessary.
statinfo = os.stat(new_path)
if not statinfo.st_mode & stat.S_IWRITE:
os.chmod(new_path, statinfo.st_mode | stat.S_IWRITE)
Comment on lines +220 to +223
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can also be found in auditwheel

# Delocate this file now that it is stored locally.
needs_delocating.add(new_path)
# Update out_lib_dict with the new file paths.
Expand All @@ -224,7 +236,7 @@ def _update_install_names(
for required in files_to_delocate:
# Set relative path for local library
for requiring, orig_install_name in lib_dict[required].items():
req_rel = relpath(required, dirname(requiring))
req_rel = posix_relpath(required, dirname(requiring))
new_install_name = "@loader_path/" + req_rel
if orig_install_name == new_install_name:
logger.info(
Expand Down Expand Up @@ -401,10 +413,6 @@ def _dylibs_only(filename: str) -> bool:
return filename.endswith(".so") or filename.endswith(".dylib")


def filter_system_libs(libname: str) -> bool:
return not (libname.startswith("/usr/lib") or libname.startswith("/System"))


def _delocate_filter_function(
path: str,
*,
Expand Down Expand Up @@ -701,7 +709,7 @@ def delocate_wheel(
]
_make_install_name_ids_unique(
libraries=libraries_in_lib_path,
install_id_prefix=DLC_PREFIX + relpath(lib_sdir, wheel_dir),
install_id_prefix=DLC_PREFIX + posix_relpath(lib_sdir, wheel_dir),
)
rewrite_record(wheel_dir)
if len(copied_libs) or not in_place:
Expand Down
15 changes: 10 additions & 5 deletions delocate/libsana.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import warnings
from os.path import basename, dirname, realpath
from os.path import join as pjoin
from pathlib import Path
from typing import (
Callable,
Dict,
Expand Down Expand Up @@ -40,7 +41,11 @@ class DependencyNotFound(Exception):
"""


def _filter_system_libs(libname: Text) -> bool:
def filter_system_libs(libname: Text) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was duplicated between here and delocating.py, so I removed the latter in favor of this copy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moving filter_system_libs like this technically breaks the API. Were you using it in your own library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's a good point. Given the circular import issues between these to files, I guess I can just revert back to the duplication.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is really showing its age by accepting a Text input. I suspect it actually works with POSIX paths exclusively. Did something happen to cause this refactor?

I've very hesitant to accept a PR with os.path.splitdrive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these changes are the result of repairing macOS wheels on a Windows host.

If I remember correctly, callers of filter_system_libs are inquiring about, for example, C:\usr\lib\libSystem.B.dylib - which obviously doesn't exist on the repairing host - but we can still consider it as a system library. So maybe the proper fix is elsewhere upstream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm very interested in learning what's putting C: into C:\usr\lib\libSystem.B.dylib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to reproduce the issue again tomorrow. Unfortunately I don't actually have a Windows machine so it's a lot of pushing and waiting for CI. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, since you've reopened this:

I'm very interested in learning what's putting C: into C:\usr\lib\libSystem.B.dylib.

It comes from here. On windows, realpath("foo") == "C:\\foo", where C:\ is the drive letter or UNC path of the current working directory. So if you chdir into \\myserver\shared_dir, then realpath("foo") == "\\\\myserver\\shared_dir\\foo"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good work. I think realpath should not be called on non-existing files in this case.

If I changed the return types for search_environment_for_lib then I'd use Path for existing files and PurePosixPath for missing files.

"""Return True if libname starts with /System or /usr/lib."""
# If libname has a drive letter, we need to strip it off first.
_, libname = os.path.splitdrive(libname)
libname = Path(libname).as_posix()
return not (libname.startswith("/usr/lib") or libname.startswith("/System"))


Expand Down Expand Up @@ -91,7 +96,7 @@ def get_dependencies(
logger.debug("Ignoring dependencies of %s" % lib_fname)
return
if not os.path.isfile(lib_fname):
if not _filter_system_libs(lib_fname):
if not filter_system_libs(lib_fname):
logger.debug(
"Ignoring missing library %s because it is a system library.",
lib_fname,
Expand All @@ -111,7 +116,7 @@ def get_dependencies(
else:
dependency_path = search_environment_for_lib(install_name)
if not os.path.isfile(dependency_path):
if not _filter_system_libs(dependency_path):
if not filter_system_libs(dependency_path):
logger.debug(
"Skipped missing dependency %s"
" because it is a system library.",
Expand Down Expand Up @@ -327,7 +332,7 @@ def _tree_libs_from_libraries(
def tree_libs_from_directory(
start_path: str,
*,
lib_filt_func: Callable[[str], bool] = _filter_system_libs,
lib_filt_func: Callable[[str], bool] = filter_system_libs,
copy_filt_func: Callable[[str], bool] = lambda path: True,
executable_path: Optional[str] = None,
ignore_missing: bool = False,
Expand Down Expand Up @@ -754,7 +759,7 @@ def wheel_libs(
When dependencies can not be located and `ignore_missing` is False.
"""
if filt_func is None:
filt_func = _filter_system_libs
filt_func = filter_system_libs
with TemporaryDirectory() as tmpdir:
zip2dir(wheel_fname, tmpdir)
lib_dict = tree_libs_from_directory(
Expand Down