bobfang1992 / pytomlpp

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

Build wheels for Mac OS (non-pypy) #14

Closed chaitan94 closed 4 years ago

chaitan94 commented 4 years ago

Currently, this fails with the following error:

   [ 50%] Building CXX object CMakeFiles/pytomlpp.dir/src/pytomlpp.cpp.o
  In file included from /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pip-req-build-ojfeoq17/src/pytomlpp.cpp:3:
  /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pip-req-build-ojfeoq17/include/pytomlpp.hpp:38:99: error: 'value' is unavailable: introduced in macOS 10.14
          py::object time_delta = PY_DATETIME_MODULE.attr("timedelta")("minutes"_a = dt.time_offset.value().minutes);
                                                                                                    ^
  /Applications/Xcode_11.5.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/optional:943:33: note: 'value' has been explicitly marked unavailable here
      constexpr value_type const& value() const&
                                  ^
  1 error generated.

This seems to be because of the C++17 feature optional behaving differently on MacOS.

bobfang1992 commented 4 years ago

Hi thanks for the PR, I do not have my Mac with me at this moment. I will review this later. Will keep you posted on what I may find.

bobfang1992 commented 4 years ago

Okay I got this working -- I replaced std::optional with a customized tl::optimal (recommended by tomlplusplus on their webpage) and this seems now working. @chaitan94 @EpicWink do you see any potential issue going forward with this?

chaitan94 commented 4 years ago

Can't really say for sure as I haven't had much experience with C++17 features yet. But I think we should be good as long as our tests run on all platforms.

EpicWink commented 4 years ago

Try building the package in a manylinux1 Docker image, if you're particularly worried. Otherwise I agree: if it builds in Mac CI, then there should be no issues

bobfang1992 commented 4 years ago

Cool thanks I will get this merged then.