build windows with conan support

[nilsnolde]

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]

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]

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]

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]

urgh.. it worked so far, but seems we're running into pybind/pybind11#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]

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]

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]

ah, didn't include that file yet, but would be helpful no?

[nilsnolde]

I added it

[nilsnolde]

I still think it can't hurt to have it here for the old people like me who are still used to python setup.py xx;) happy to remove it as well though

[nilsnolde]

this will instruct conan to write the build information of the 2 libraries to a json which we can then parse in the setup.py

[nilsnolde]

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

[jonathf]

Amazing job, Nils!

A little bit of cleanup and this should be ready to merge.

[jonathf]

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]

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

[nilsnolde]

actually, using conan on mac for ci would remove these lines as well. which are not very robust I think. brew is pretty updated always I found, some users might actually have 3.x soon? anyways, more of a works-now-why-change thing. but maybe good to keep in mind in worst case

[nilsnolde]

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]

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:

• we need the path to cache for conan's home (that's where the libs live). it can be hard-coded (it's at C:\Users\runneradmin\.conan), but not even that works..
• cache the folder conan installs the aux files to (./conan_build) as per conan_install.bat

[jonathf]

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

[jonathf]

Sorry, wrong button...

[nilsnolde]

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]

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..

[jonathf]

Great work @nilsnolde! I know how difficult win build can be.