Skip to content

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

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.

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

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Jul 11, 2023

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

@ikappaki ikappaki force-pushed the issue/win-support branch 3 times, most recently from cceaf3d to 3055842 Compare July 11, 2023 21:38
Also
- Add ci test matrix across different archs, java and python versions.
- Fix a test issue with name reference in the python collections lib.
@ikappaki ikappaki force-pushed the issue/win-support branch from 3055842 to 5667374 Compare July 12, 2023 06:34
@jjtolton
Copy link
Contributor

jjtolton commented Jul 12, 2023

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.

Copy link
Contributor

@jjtolton jjtolton left a comment

Choose a reason for hiding this comment

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

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

@ikappaki
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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

To be considered to go to seperate GH issues/PRs instead
@ikappaki
Copy link
Contributor Author

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 jjtolton merged commit b03b4f1 into clj-python:master Jul 14, 2023
@jjtolton
Copy link
Contributor

Thank you for this, @ikappaki !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to find a valid python library on MS-Windows with the official python distributions
2 participants