search

VROOM-Project / pyvroom

Vehicle Routing Open-source Optimization Machine
BSD 2-Clause "Simplified" License
69 stars 14 forks source link

build windows with conan support #9

Closed nilsnolde closed 2 years ago

nilsnolde commented 2 years ago

fixes #4

still WIP, didn't try on windows yet. worked on linux though. windows worked as well as on linux. so anyone should be able to use conan if desired.

one thing I realized: at least on linux conan installs only static libraries for openssl & crypto, so the final python .so is bigger but not much to do for auditwheel then.

jonathf commented 2 years ago

Cool.

Just so you are aware, cibuildwheels already does this. On linux it uses auditwheel and on macos it uses Delocate (I think).

Considering the short list of dependencies, I much prefer static linking.

jonathf commented 2 years ago

If you change the branch trigger from master (which does not exist) to nn-add-win-builds, I belive I should get a prompt about allowing CI to kick in for your builds. When you think it is time.

nilsnolde commented 2 years ago

the changes in the actions yaml are up for discussion for sure. indeed my muscle memory kicked in with master. but it's fine in the pull_request section, then it should kick in as well.

nilsnolde commented 2 years ago

urgh.. it worked so far, but seems we're running into https://github.com/pybind/pybind11/issues/1908. running the example in ipython:

c:\users\nilsn\documents\dev\python\pyvroom\venv\lib\site-packages\vroom\input\input.py in set_durations_matrix(self, profile, matrix_input)
     88         assert isinstance(profile, str)
     89         if not isinstance(matrix_input, Matrix):
---> 90             matrix_input = Matrix(numpy.asarray(matrix_input, dtype="uint32"))
     91         _Input.set_durations_matrix(self, profile, matrix_input)

RuntimeError: Incompatible buffer format!

I didn't read the full issues, it's quite long, so there might be a solution/workaround somewhere. at least it worked locally.. pushing soon.

jonathf commented 2 years ago

Ah, yeah I am familiar with the issue.

We need a smarter way to determine the dtype in vroom.Matrix initializers. Either by selecting it in a smarter way, or by doing some sort of platform check.

Shouldn't happen too often. Right now it should only be vroom.Matrix and vroom.Amount which I pushed Yesterday.

nilsnolde commented 2 years ago

Ah, yeah I am familiar with the issue.

ok great!

that should be it now I hope. I'll comment more in the PR. just one thing I don't quite get yet: how does cibuildwheel know what's in [build-system].requires of the pyproject.toml? is there some magic going on inside cibuildwheel which uses that value? locally I had to install pybind etc in my virtual env (I have a build-requirements.txt for pip I thought I'd contribute). but then CI obviously works, just don't understand how;)

nilsnolde commented 2 years ago

I'll see tmrw if the cache for conan worked as well this time

jonathf commented 2 years ago

that should be it now I hope. I'll comment more in the PR. just one thing I don't quite get yet: how does cibuildwheel know what's in [build-system].requires of the pyproject.toml? is there some magic going on inside cibuildwheel which uses that value? locally I had to install pybind etc in my virtual env (I have a build-requirements.txt for pip I thought I'd contribute). but then CI obviously works, just don't understand how;)

PEP 517 is a python specification for placing it there. Most installers like pip install, python -m build, poetry install, etc. recognizes it.

jonathf commented 2 years ago

BTW, I think you could get python -m build (which is what cibuildwheel uses) to work by adding conanbuildinfo.json to MANFEST.in.

nilsnolde commented 2 years ago

addressed your review, thanks, was really helpful!

win CI is not caching right yet. might need to do a round or two more.. generally no stress with this (from my side anyways). I seem super active right now, but just trying to get it out of my head;)

nilsnolde commented 2 years ago

jeez.. that ruined my day really, as much as I hate letting things go, this is one of these times.. for now anyways..

env vars in github actions are TERRIBLE.. I really tried 3-4 different ways of doing it, none worked (also doesn't help that it's not really stable yet and keeps changing).. this is the official guide: https://docs.github.com/en/actions/learn-github-actions/workflow-commands-for-github-actions#environment-files. no idea how to work these environment files and the section below that (>> $GITHUB_ENV) didn't work either.

if you have a good idea, most appreciated.. the problem is that:

jonathf commented 2 years ago

Sorry, I have very little expertice on Windows. My guess is likely going to be worse than yours.

jonathf commented 2 years ago

Sorry, wrong button...

nilsnolde commented 2 years ago

no worries, next time I'll have more patience. problem is github actions, not so much windows. it just won't cache the path I tell it to. win desktop is one frustrating experience, but windows CI takes the cake!

nilsnolde commented 2 years ago

oh boy... had a long battle with win ci in our fork.. seems like I slayed the bastard finally!

problems were many.. the cache action somehow uses gnu tar.exe which seems to have problems with windows path separators. and github actions env vars on windows have to be prefixed with $env:, e.g. $env:GITHUB_WORKSPACE. then there's 3 different shells to use, each worse than the other and and and..