matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
262 stars 59 forks source link

Tweak import to make vendoring tool happy #200

Closed jvolkman closed 4 months ago

jvolkman commented 4 months ago

I vendor delocate as part of repairwheel, and much of the patch set that I maintain is relativizing these imports to make vendoring easier.

HexDecimal commented 4 months ago

That's a hit to readability for sure.

I don't think this is standard. Why patch the imports compared to say, adding the vendor directory to sys.path?

jvolkman commented 4 months ago

Upon further investigation, vendoring actually converts imports itself and already handles most of the things I was patching. There was one import that it wasn't happy about, so I've left that and removed the rest of the changes.

HexDecimal commented 4 months ago

Why doesn't it like that specific import?

jvolkman commented 4 months ago

Why doesn't it like that specific import?

In its own words:

  Encountered import that cannot be transformed for a namespace.
  File "src/repairwheel/_vendor/delocate/libsana.py", line 23
    import delocate.delocating

  You will need to add a patch, that adapts the code to avoid a `import dotted.name` style import here; since those cannot be transformed for importing via a namespace.
HexDecimal commented 4 months ago

Thanks for looking into it. I can live with this version of the changes.

I'll try my best to avoid this "plain dotted imports" syntax in the future.

Looks the odd import here was out of necessity to handle a cyclic import. DelocationError can be moved to a better spot.

jvolkman commented 4 months ago

Maybe DelocationError can just move to libsana.py? delocate.py already imports other stuff from libsana.

HexDecimal commented 4 months ago

I'm not sure. Putting the exceptions in a new module without any internal imports is probably the best option, but I'm okay with whatever resolves the cyclic import.

codecov[bot] commented 4 months ago

Codecov Report

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

Comparison is base (1e9095c) 96.81% compared to head (4576698) 96.81%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #200 +/- ## ========================================== - Coverage 96.81% 96.81% -0.01% ========================================== Files 15 15 Lines 1289 1288 -1 ========================================== - Hits 1248 1247 -1 Misses 41 41 ```

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

jvolkman commented 4 months ago

@HexDecimal I guess this change also breaks the public API. Does it matter? Are you aware of people using this project as an API vs. its CLI?

HexDecimal commented 4 months ago

DelocationError is still used in both modules, so nothing would break at runtime at least.

The public API is something @matthew-brett cares about more than I do. I wanted to take more advantage of the project being pre-1.0.0. People using the the library API is mostly speculation as far as I know. I think you're the only one I know using the API, which makes me even less supportive of a stable API since any real downstream projects should be having more of a say in the API at this state of development.

That said. I'm leaning towards eventually breaking the current API while following Semantic Versioning rules. Which would allow a 0.11.0 release with a refactored API. Updating all the semi-public internals to use pathlib would help with maintainability a lot. For now though I just shout at anyone not marking their new functions as private.