Closed chaitan94 closed 4 years ago
Windows should be trivial for this, I don't understand what's so tricky?
The GitHub Actions images include CMake and Visual Studio for Python 3.5 and later, so once you drop 2.7 support it ought to be straightforward.
Do some of the build tools you're using need contributions?
I would suggest you pull in the changes of #2, That would remove the need for Cmake, especially as then we're testing just the library itself. This could also fix the issues with Windows
Looking at this, and I think I agree with @EpicWink on where this should go: we should remove cmake altogether and just use python setup.py/pip to build this package. Also can remove the google tests as there is no need for them. @chaitan94 Do you want to make this change? Thanks!
I guess to do is to:
And then we are in good shape.
Sure will make suggested changes.
Apart from that are any of you aware of what could be the issue I'm facing in Mac OS build? I am not familiar with C++17 features as much yet. Seems like optional
feature is not working as expected in mac.
@chaitan94 I will look into that and see if I can help you.
@chaitan94 I merged #3, please rebase, thanks.
Regarding google tests, I guess we wouldn't need test_lib_config.cpp
as well right?
Regarding google tests, I guess we wouldn't need
test_lib_config.cpp
as well right?
yep, no need for it anymore @chaitan94
I've pulled in all latest changes and removed google-tests related code. Right now wheels are being built both by Github Actions and Travis (Although we have yet to fix the macOS issue on Github Actions). Should we deprecate Travis?
Yeah, let's deprecate Travis. Give me some time and I will see if I can fix the Mac OS issue. BTW is windows okay now or you need to work on it?
Right now its failing with 'unknown option '-std=c++17', I think MSVC expects something like '/std:c++17' instead of '-std=c++17'. I will try that and check accordingly.
PyPy build also seems to fail (even on Linux): https://github.com/chaitan94/pytomlpp/runs/790601180?check_suite_focus=true
In file included from src/pytomlpp.cpp:3:
include/pytomlpp.hpp: In function ‘toml::array pytomlpp::py_list_to_toml_array(const pybind11::list&)’:
include/pytomlpp.hpp:264:77: error: cannot bind non-const lvalue reference of type ‘pybind11::handle&’ to an rvalue of type ‘pybind11::handle’
toml::date_time date_time_value = py_datetime_to_toml_date_time(it);
^~
I have added back toml-test. I also made a change so that tests won't succeed without it.
Guys I have restricted to Linux and non-pypy. I think it would be best if we merge the work so far and continue work on fixing Mac OS, Windows and PyPy build seperately. Since there are many commits already constantly rebasing would be a inefficient use of time.
The build and test is succeeding as of now: https://github.com/chaitan94/pytomlpp/runs/790658356?check_suite_focus=true
Hi, Thanks for all the hard work. So the gist is:
Is this understanding correct? I will be happy to merge this if it is. And then we can work on separate PRs to get the above limitations removed.
Just 2 points:
Thanks!
Hi, Thanks for all the hard work. So the gist is:
* This change deprecates Travis and uses Github Actions instead. * it builds on Linux * it does not build on Windows or Mac OS yet, also not working for Pypy
Is this understanding correct? I will be happy to merge this if it is. And then we can work on separate PRs to get the above limitations removed.
Just 2 points:
1. Let me do a quick review 2. Could you rebase? Seems currently we have conflicts.
Thanks!
Yes, you are correct on all three points. I have resolved the conflicts just a few minutes ago - there shouldn't be any now. I have remove cmake realted stuff. We should be good to merge once this suceeds: https://github.com/chaitan94/pytomlpp/actions/runs/141790648 .
Windows should be trivial for this, I don't understand what's so tricky?
@zooba Windows is ALWAYS the trickiest. Never underestimate it :stuck_out_tongue_closed_eyes:.
Hence, let's keep it and macos support separate from this for now.
Hi, sorry I still see there is conflicts and cannot merge... Can you check again? @chaitan94
This branch cannot be rebased due to conflicts Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.
This is what I am seeing right now.
Maybe you want to squash and merge instead?
Maybe you want to squash and merge instead?
You can do this as well
git fetch -p --all
git reset --soft upstream/master
git add -u
git commit
This branch cannot be rebased due to conflicts
@bobfang1992 I made a clean rebase again now. Hopefully, you shouldn't face any issues now.
Ahh yeah, pathlib.Path.glob
is a generator. You'll need to valid_toml_files = list(valid_toml_files)
Great thanks! This is a lot of work. Thank you a lot @chaitan94
Wheels building status
Publishing workflow
Note: We do not build and publish sdist to pip. Ideal publishing flow for any new release should be:
(Assuming we are publishing a version say 1.2.3)
Publish to test PyPI
python setup.py sdist
twine upload --repository testpypi dist/*
pip install -i https://test.pypi.org/simple/ pytomlpp==1.2.3rc1
Publish to PyPI
(First make sure PYPI_TOKEN is setup in your repository's Github secrets)
Bump to 1.2.3
git push
git tag 1.2.3
git push origin 1.2.3