Tweaks for Windows portability #201

jvolkman · 2024-02-06T04:18:57Z

This is the other half of the patch set I've been maintaining for repairwheel. Hopefully these changes are innocuous enough to push upstream.

Don't assume we're running on a system with POSIX-style paths.

jvolkman · 2024-02-06T04:20:55Z

delocate/delocating.py

+        # 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)


This can also be found in auditwheel

jvolkman · 2024-02-06T04:21:51Z

delocate/libsana.py

@@ -40,7 +41,11 @@ class DependencyNotFound(Exception):
    """


-def _filter_system_libs(libname: Text) -> bool:
+def filter_system_libs(libname: Text) -> bool:


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

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

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.

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.

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.

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

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. :(

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"

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.

HexDecimal · 2024-02-06T04:24:50Z

Honestly I think the whole source should be converted to use pathlib's Path and PurePosixPath.

HexDecimal · 2024-02-06T04:39:28Z

This kind of stuff I want to revisit by adding Ruff's PTH rule. I don't want these outdated calls to be refactored into other outdated calls.

jvolkman · 2024-02-06T04:52:31Z

Moving to pathlib sounds great.

I was interested to see that there are CI tests running on windows. I don't know the full history of this project but I guess there was some cross-platform consideration at some point.

HexDecimal · 2024-02-06T04:57:51Z

I was interested to see that there are CI tests running on windows. I don't know the full history of this project but I guess there was some cross-platform consideration at some point.

There are a few issues such as #29 and some others about using cross platform tools. I added Windows testing at least to keep track of which features were portable, but most of them rely on either MacOS tools or linkage.

HexDecimal

Recommended reading: https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module

I don't think now is the time for a major refactor. Just fix what's absolutely needed.

HexDecimal · 2024-02-06T05:27:29Z

delocate/delocating.py

+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()


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

HexDecimal · 2024-02-06T05:31:05Z

delocate/delocating.py

@@ -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:


Suggested change

if Path(rp_root_path) not in Path(required).parents:

if not Path(required).is_relative_to(rp_root_path):

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

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("..").

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.

HexDecimal · 2024-02-06T05:44:26Z

delocate/libsana.py

@@ -40,7 +41,11 @@ class DependencyNotFound(Exception):
    """


-def _filter_system_libs(libname: Text) -> bool:
+def filter_system_libs(libname: Text) -> bool:


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.

HexDecimal · 2024-02-06T07:04:04Z

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. :(

True Windows support is going to be a difficult task. It'll be far more complex than making sure paths work correctly. I suggest closing this for now and not worrying about Windows support for the foreseeable future.

HexDecimal · 2024-02-07T23:07:00Z

I apologize, but I'd like to put any Windows specific refactoring on hold for now. At least until certain Ruff rules are enforced and older versions of Python are dropped.

HexDecimal · 2024-02-07T23:09:23Z

You can still make a PR for refactors needed by repairwheel, if necessary.

jvolkman · 2024-02-07T23:21:58Z

True Windows support is going to be a difficult task. It'll be far more complex than making sure paths work correctly.

This is true, but I've already tackled the more complicated stuff in repairwheel by reimplementing otool, install_name_tool, and codesign in Python. But I'm able to more easily patch those changes in at runtime (here).

Unless you're talking about delocating Windows dlls, for which there is already delvewheel.

HexDecimal · 2024-02-07T23:47:33Z

I've already tackled the more complicated stuff in repairwheel by reimplementing otool, install_name_tool, and codesign in Python.

Then you've already finished the hardest parts. Never mind my pessimism, it sounds like you've got it handled.

The reason I closed this is because your changes were causing readability and future refactoring concerns, such as is_resolved_subpath not being documented as a workaround to a missing Path.is_relative_to. I want to avoid the code being full of posix_relpath and is_resolved_subpath style workarounds. This would likely mean dropping Python 3.8 support before continuing, and adding linter rules to enforce the use of pathlib. These updates would conflict with this PR.

I'm handing another large PR that I'd like to merge before working on this one.

I'll reopen this to keep track of it, but it's possible that you may have to start over at a later time.

codecov · 2024-02-07T23:53:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eaba43d) 96.45% compared to head (8ffe7ac) 96.47%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   96.45%   96.47%   +0.02%     
==========================================
  Files          15       15              
  Lines        1184     1192       +8     
==========================================
+ Hits         1142     1150       +8     
  Misses         42       42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Tweaks for Windows portability

8faed8b

Don't assume we're running on a system with POSIX-style paths.

jvolkman commented Feb 6, 2024

View reviewed changes

jvolkman marked this pull request as ready for review February 6, 2024 04:23

HexDecimal requested changes Feb 6, 2024

View reviewed changes

More fixes

8ffe7ac

HexDecimal closed this Feb 7, 2024

HexDecimal reopened this Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaks for Windows portability #201

Tweaks for Windows portability #201

jvolkman commented Feb 6, 2024 •

edited

Loading

jvolkman Feb 6, 2024

jvolkman Feb 6, 2024

HexDecimal Feb 6, 2024

jvolkman Feb 6, 2024

HexDecimal Feb 6, 2024

jvolkman Feb 6, 2024

HexDecimal Feb 6, 2024

jvolkman Feb 6, 2024

jvolkman Feb 7, 2024

HexDecimal Feb 8, 2024

HexDecimal commented Feb 6, 2024

HexDecimal commented Feb 6, 2024

jvolkman commented Feb 6, 2024

HexDecimal commented Feb 6, 2024

HexDecimal left a comment

HexDecimal Feb 6, 2024

HexDecimal Feb 6, 2024

jvolkman Feb 6, 2024

HexDecimal Feb 6, 2024

jvolkman Feb 6, 2024

HexDecimal Feb 6, 2024

HexDecimal commented Feb 6, 2024

HexDecimal commented Feb 7, 2024

HexDecimal commented Feb 7, 2024 •

edited

Loading

jvolkman commented Feb 7, 2024

HexDecimal commented Feb 7, 2024

codecov bot commented Feb 7, 2024

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

Tweaks for Windows portability #201

Are you sure you want to change the base?

Tweaks for Windows portability #201

Conversation

jvolkman commented Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HexDecimal commented Feb 6, 2024

HexDecimal commented Feb 6, 2024

jvolkman commented Feb 6, 2024

HexDecimal commented Feb 6, 2024

HexDecimal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HexDecimal commented Feb 6, 2024

HexDecimal commented Feb 7, 2024

HexDecimal commented Feb 7, 2024 • edited Loading

jvolkman commented Feb 7, 2024

HexDecimal commented Feb 7, 2024

codecov bot commented Feb 7, 2024

Codecov Report

jvolkman commented Feb 6, 2024 •

edited

Loading

HexDecimal commented Feb 7, 2024 •

edited

Loading