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

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

Merged
merged 4 commits into from
Nov 18, 2021
Merged

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Nov 18, 2021

This is a result of the pypy failure in the conda feedstock. The problem with pybind11 is tracked here pybind/pybind11#3408 (comment)

@bobfang1992
Copy link
Owner

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?

src/encoding_decoding.cpp Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented Nov 18, 2021

I fixed the formatting and removed the debug cruft.

@mattip
Copy link
Contributor Author

mattip commented Nov 18, 2021

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

@bobfang1992 bobfang1992 merged commit 9dbe2f5 into bobfang1992:master Nov 18, 2021
@mattip
Copy link
Contributor Author

mattip commented Nov 18, 2021

Thanks!

@henryiii
Copy link

henryiii commented Nov 22, 2021

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

# Additionally, skip 32-bit Windows for now as MSVC needs seperate setup with different toolchain to do this
# Refer: https://cibuildwheel.readthedocs.io/en/stable/cpp_standards/#windows-and-python-27
CIBW_SKIP: cp27-* pp* *-win32 cp35-*
CIBW_SKIP: cp27-* *-win32 cp35-*

Choose a reason for hiding this comment

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

You can remove the skipping of cp27 & cp35 since cibuildwheel 2.0 (in fact, it should be printing a warning stating these are not needed).

Copy link
Owner

Choose a reason for hiding this comment

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

yeah that's for the suggestion, but I think we intentionally skipped cp27, because the underlying C++ library won't compile and python2.7 is reaching end of life. But I cannot remember clearly why we skipped cp35 though. But I am pretty sure certain it was because an issue, though I am not sure what the issue was. Do you have a need for these two to work? If so I can take a look later probably.

Choose a reason for hiding this comment

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

Cibuildwheel 2.0 dropped Python 2.7 and Python 3.5, so there's no need to skip them anymore, is what I meant. :)

Copy link
Owner

Choose a reason for hiding this comment

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

Ah make sense! Thanks for letting me knnow.

bobfang1992 added a commit that referenced this pull request Nov 22, 2021
#61 suggests that these tags are not needed.
@bobfang1992 bobfang1992 mentioned this pull request Nov 22, 2021
@henryiii
Copy link

henryiii commented Nov 22, 2021

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)

bobfang1992 added a commit that referenced this pull request Nov 22, 2021
#61 suggests that these tags are not needed.
@henryiii
Copy link

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

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

@henryiii
Copy link

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

@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!

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.

4 participants