delocate icon indicating copy to clipboard operation
delocate copied to clipboard

Tweaks for Windows portability

Open jvolkman opened this issue 1 year ago • 10 comments

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

jvolkman avatar Feb 06 '24 04:02 jvolkman

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

HexDecimal avatar Feb 06 '24 04:02 HexDecimal

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.

HexDecimal avatar Feb 06 '24 04:02 HexDecimal

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.

jvolkman avatar Feb 06 '24 04:02 jvolkman

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 avatar Feb 06 '24 04:02 HexDecimal

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 avatar Feb 06 '24 07:02 HexDecimal

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 avatar Feb 07 '24 23:02 HexDecimal

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

HexDecimal avatar Feb 07 '24 23:02 HexDecimal

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.

jvolkman avatar Feb 07 '24 23:02 jvolkman

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.

HexDecimal avatar Feb 07 '24 23:02 HexDecimal

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 07 '24 23:02 codecov[bot]