search

clj-python / libpython-clj

Python bindings for Clojure
Eclipse Public License 2.0
1.05k stars 68 forks source link

Fix to correctly locate python DLL on MS-Windows #247

Closed ikappaki closed 12 months ago

ikappaki commented 12 months ago

Hi,

can you pleaser review patch to properly locate the official python dll library on MS-Windows. It fixes #246.

The python dll name on MS-Windows does not have . in its name as it is the case with the other platforms. This just adds a version of the library to look out for without the ..

In the meantime, I've noticed that there are no cross platform CI tests on the codebase (travis config appears to be outdated and limited ubuntu only), and though to add an example github workflow to run the unit tests across macos, win and Linux. I've only included the latest official python version (3.11) for economy, while extending the test coverage to at least 3 jdk versions (8, 17 and 19) which I thought as being important coverage. Let me know what you think, happy to revert or further modify.

I've also updated one of the test to import Mappings from collections to collections.abc as it fails in python versions > 3.9 (see https://stackoverflow.com/questions/70195545/module-collections-has-no-attribute-mapping-issue-on-macos-for-sdk-installa for reference)

Thanks

jjtolton commented 12 months ago

Over looks good! Thank you @ikappaki. I only have a few small cleanup comments. @cnuernber could you comment on the inclusion of this file? Not sure how it fits in with the overall repo management.

ikappaki commented 12 months ago

Overall looks good! Could you please either revert or explain those two changes in the .gitignore file and the python_test.clj file?

Hi @jjtolton , thanks for the thorough review. I've tried explaining these two in their respective comments, I can revert .gitignore at your discretion, but python_test.clj change is essential for running the tests with python 3.10 and above.

ikappaki commented 12 months ago

Hi @jjtolton, just to confirm the plan in terms of Issues and PRs to raise and submit going forward as per your suggestion

  1. An Issue and PR for the DLL issue (this one)
  2. An Issue and PR for the test fix
  3. An Issue and PR for the CI coverage upgrade
  4. An Issue and PR for the Emacs backup file exclusion

thanks

jjtolton commented 12 months ago

Perfect. I'd be comfortable bundling the test job and the test fix into one (items 2 and 3).

ikappaki commented 12 months ago

Perfect. I'd be comfortable bundling the test job and the test fix into one (items 2 and 3).

I've reverted the test fix, CI suggestion and .gitignore change, and moved the first two in #249.

Thanks

jjtolton commented 12 months ago

Thank you for this, @ikappaki !