Skip to content
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

Fixed all warnings with recent Pythons and when compiling with C++17 #6

Merged
merged 21 commits into from
Sep 5, 2024

Conversation

pbosetti
Copy link
Contributor

Fixed deprecation warnings on modern pythons.

Also, fixed deprecation warning when compiling with C++17

@reefactor
Copy link
Owner

Hey @pbosetti thanks for PR, I've been long striving for a free time to fix Windows platform build issues and autotests

Can you help me fix github pipeline for windows https://github.com/reefactor/cppy3/actions/runs/10662698254
There's Python3_NumPy_INCLUDE_DIRS missing

That's because numpy package missing in github workflow matrix file https://github.com/reefactor/cppy3/blob/master/.github/workflows/cmake-multi-platform.yml#L72

Can you give a hint or a code commit on how to setup a numpy visibility for CMake on Windows ?

CMake Error at C:/Program Files/CMake/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found
  suitable version "3.12.5", minimum required is "3.5")
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindPython/Support.cmake:4002 (find_package_handle_standard_args)
  C:/Program Files/CMake/share/cmake-3.30/Modules/FindPython3.cmake:596 (include)
  CMakeLists.txt:[21](https://github.com/reefactor/cppy3/actions/runs/10662698254/job/29559451107#step:7:22) (find_package)

@pbosetti
Copy link
Contributor Author

pbosetti commented Sep 2, 2024

Hi, thanks for answering... I supposed the project was dead or so, and I went on adapting it to my own needs. In the last commits I probably have changed it too much for your liking... Anyway, after a day of fighting Windows pesky specifics, I finally have it working (gosh, If I hate that platform...)

Now I have it working after installing official Python 3.12, selecting "install for all users of this PC". Then I just did a pip install numpy, and it compiles fine, both with and without CPPY3_BUILD_EXECUTABLES, so I suppose it's just a matter of adding this command to the workflow.
I have not focused (yet) on the unit tests: just now I have run the tests executable and it fails one test case on multibyte chars (grrrr). I will dedicate more time to this problem in the next days and report here.

@pbosetti
Copy link
Contributor Author

pbosetti commented Sep 2, 2024

Oh, I was forgetting: I have by default disabled the compilation of tests: if you want to enable tests (as in the workflow), you need to -DCPPY3_BUILD_EXECUTABLES=ON. That's why CI fails also on Linux and macOS right now.

Reason of this choice: I suppose most of the times this project is going to be used as library, included with FetchContent: in these conditions, it makes little sense to also build the executables by default.

@pbosetti
Copy link
Contributor Author

pbosetti commented Sep 2, 2024

Hi, in the last group of commits I have tried to fix all warnings and Windows issues, at least when compiling with MSVC19 with C++17 standard.
There is still a failing test on utils.cpp, where it fails in string conversions. I'd still struggling to understand why, so — as a temporary solution — I have added a cmake option CPPY3_TEST_UNICODE_CONVERTERS to skip that section of the test suite (default is ON). With this, it should suffice to set the option in the proper section of cmake-multi-platform.yml.

@reefactor
Copy link
Owner

reefactor commented Sep 3, 2024

How do you set Python3_NumPy_INCLUDE_DIRS CMake variable on windows ?

I need to add this into workflow file

to fix CI for Windows platform https://github.com/reefactor/cppy3/actions/runs/10669260589/job/29635962034?pr=6

Linux and Mac CI tests are GREEN passed now.

@pbosetti
Copy link
Contributor Author

pbosetti commented Sep 4, 2024

Damn, that's tough. If I run python3 -c "import numpy; import os; print(os.path.dirname(numpy.__file__) + '/core/include')" on GH pipeline I get: C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\numpy\core\include, which Is correct, it's where NumPy is installed. But it won't work, still.
There is something different in GH Windows install wrt my own clean install, so that I cannot replicate the issue on my windows machine. This makes it hard to understand what's not working.
Any ideas?

@pbosetti
Copy link
Contributor Author

pbosetti commented Sep 4, 2024

@reefactor I have invited you on a test repo that only has the bare basics for GH CI. Look at the pipeline: the step Python path on output explains the failure: the directory containing NumPy lacks the include dir, which on the contrary is present on a clean install.
Now I am not an expert in Python (I actually despise it), so you probably can illuminate me on this issue, can't you?

@reefactor reefactor merged commit 501a49a into reefactor:master Sep 5, 2024
7 of 9 checks passed
@reefactor
Copy link
Owner

reefactor commented Sep 5, 2024

@pbosetti many thanks for your contribution, I appreciate the code - it is cleaner and its C++17 now. Thats a major step for this small package.

And we've managed to fix Windows platform build.

It remains to be diagnosed and fixed Windows autotests: Debug build python linking issue and Release build segfault.

Release tests crashing on Windows is a mystery for now and requires debugging.
Debug Linking with debug python on Windows is a known nightmare, needs more time.

https://github.com/reefactor/cppy3/actions/runs/10724591615/job/29740578852?pr=6

Seems like it requires more time and effort and should be delayed for now.

@reefactor
Copy link
Owner

@pbosetti regarding your remark on despising python - since you're familiar with Ruby - you should get familiar with Python more. All the machine learning engineering & research math community folks using it and pretty happy.

@pbosetti
Copy link
Contributor Author

pbosetti commented Sep 6, 2024

Good!
Regarding the outstanding failures:

  • The python version installed on GH instances does not have the debug library (this is what the Windows installer does by default), that's why it does not link. There's little that we can do about that, except either disable debug build on Windows (it is of little use by the way), or manually install a complete Python version earlier in the pipeline (but it's a lot of work and, again, I doubt that it's worth it). So my suggestion is to disable debug build on Windows.
  • Regarding the tests.exe segfault, this is unexpected: on my Win11 test machine it fails 1 of the 2 test cases and 1 of the assertions (due to WideToUTF8()), but it does not crash. I have no clue why it segfaults on GH instance, and I suspect it's gonna be hard to debug, again. Yet, that failed test needs to be investigate, perhaps solving it would also fix the segfault.

<rant mode="on">
And it's not that I am not familiar with Python. I just find it lacking elegance and consistency. I hate the lack of a block close mark, I hate the fact that class implementation seems an afterthought. I don't like its C bindings, and the way you have to manage with language versions, libraries, pip, conda and company it's frankly a nightmare. I hate the fact that it killed other, nicer languages with more consistent ecosystems, just because it happened to be in the right place on the right moment (but this is a rather common story in the IT world).
But my main problem with Python, as an educator, is what it does to young programmers: their whole world lives in Python. "it's such a great and powerful language! I can do anything in it! name a problem, there's a Python lib for that!"
No, it's not the language that is powerful, it's its ecosystem. Ruby (just to make an example) would have been equally powerful (but waaay more elegant), if only Matsumoto were not so stubborn and operated outside Google's environment.
And no: using a single language for any application is never a wise choice. If you only have a hammer in your toolset, then any problem is a nail.
And no: you cannot use a scripting language for any application.
And yet I'm complaining for nothing, in a few years programmers will be largely replaced by AI, and nobody will ever have to program in low-level or compiled languages; only very high level, model-description languages will survive, and in a few decades we'll have only speech interfaces. And this makes me (as one that loves programming) very much sad.
<rant mode="off">

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.

2 participants