bobfang1992 / pytomlpp

A python wrapper for tomlplusplus
https://bobfang1992.github.io/pytomlpp/pytomlpp.html
MIT License
86 stars 11 forks source link

Add PyPy to the build matrix and adapt for pypy pybind11 issue #61

Closed mattip closed 2 years ago

mattip commented 2 years ago

This is a result of the pypy failure in the conda feedstock. The problem with pybind11 is tracked here https://github.com/pybind/pybind11/issues/3408#issuecomment-972752210

bobfang1992 commented 2 years ago

Umm... let's do this, let me merge this, and I can fix the clang-format issue on my local machine, and then I can publish the new version. Happy for me to do that? Or do you prefer fixing the clang format on your side?

mattip commented 2 years ago

I fixed the formatting and removed the debug cruft.

mattip commented 2 years ago

This now builds (and tests) pytomlpp-1.0.7-pp37-pypy37_pp73-*.whl files for pypy3.7 on linux, windows, macOS.

mattip commented 2 years ago

Thanks!

henryiii commented 2 years ago

FYI, unrelated, you can use pre-commit.ci and the pip installable clang-format to automatically fix format issues on PRs (and easily run them anywhere with full version pinning). https://pypi.org/project/clang-format/ & https://scikit-hep.org/developer/style#clang-format-c-only

henryiii commented 2 years ago

Just to leave a record of what I'm doing, I stuck in the following noxfile:

import nox

PYTHON_VERISONS = ["3.6", "3.7", "3.8", "3.9", "3.10", "pypy3.7", "pypy3.8"]

@nox.session(python=PYTHON_VERISONS)
def tests(session: nox.Session) -> None:
    """
    Run the tests (requires a compiler).
    """
    session.install("-r", "tests/requirements.txt")
    session.install(".", env={"MACOSX_DEPLOYMENT_TARGET": "10.9"})
    session.run("pytest", "tests", *session.posargs)

And then nox -s tests-pypy3.7 runs the build and tests.

(MACOSX_DEPLOYMENT_TARGET is necessary for PyPy to work around the fact it tries to use 10.7, and that's been invalid since around macOS 10.14 or so due to usage of the removed stdlibc++ that occurs when this is set below 10.9)

henryiii commented 2 years ago

I don't understand why this is happening yet, but the Python way to do this also happens to work correctly on PyPy:

diff --git a/src/encoding_decoding.cpp b/src/encoding_decoding.cpp
index 0c78830..23d0057 100644
--- a/src/encoding_decoding.cpp
+++ b/src/encoding_decoding.cpp
@@ -74,16 +74,9 @@ toml::array py_list_to_toml_array(const py::list &list) {
       arr.push_back(time_value);
     } else {
       std::stringstream ss;
-#ifdef PYPY_VERSION
-      // see
-      // https://github.com/conda-forge/pytomlpp-feedstock/pull/1#issuecomment-972738986
-      // and
-      // https://github.com/pybind/pybind11/issues/3408#issuecomment-972752210
-      ss << "not a valid type for conversion " << std::endl;
-#else
-      ss << "not a valid type for conversion " << it << std::endl;
-#endif
-      throw py::type_error(ss.str());
+      throw py::type_error(py::str("not a valid type for conversion {}").format(it));
     }
   }
   return arr;

It seems to be tripping up at the compiler level, which makes me think there might be something disabled to workaround a PyPy bug that messes up the overload here.

bobfang1992 commented 2 years ago

@henryiii sorry I might be missing something here, what's the actual bug? I am failing to grasp the issue you are facing 😭

henryiii commented 2 years ago

The bug is the one avoided by this PR by the addition of a define to skip printing the object in PyPy. std::cout << it is fine on CPython, but doesn't work on PyPy for some weird reason (haven't been able to make a MWE that reproduces this). But if you just use py::str().format(), as shown above, it's fine, and that way you still get the object in the error.

bobfang1992 commented 2 years ago

@henryiii ah I see what you mean. Care to submit a PR? I think it would be great to get this in. No worries if you find it trouble-some, I can do it later. Thanks!