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

Build via setup.py proper using scikit-build #58

Merged
merged 81 commits into from
Feb 19, 2018
Merged

Build via setup.py proper using scikit-build #58

merged 81 commits into from
Feb 19, 2018

Conversation

native-api
Copy link
Contributor

@native-api native-api commented Dec 17, 2017

Subj, now it can be built with the normal setup.py build/bdist_wheel which the CI should be invoking. https://ci.appveyor.com/project/native-api/opencv-python

Things that don't yet work:

  • Didn't touch travis machinery. It's rather convoluted, could use a hand here. I still can't figure out which script is the "root" one (nothing seems to invoke docker_build_wrap.sh that looks like it according to multibuild/README.rst) and how to debug this (can't test this locally, so each edit-check step would take half a day or so).
    • Looks like deleting travis/build_* (and references) and other build steps should do since the default multibuild build fn already calls bdist_wheel.
  • sdist and test don't work yet. That I consider separate tasks.

@skvark
Copy link
Member

skvark commented Dec 18, 2017

I agree that the multibuild system is rather convoluted. The command which triggers the build is travis_wait 120 build_wheel $REPO_DIR $PLAT, build wheel is a function which becomes visible when some of the other multibuild files are sourced before that. Additionally, build_wheel has been overwritten in config.sh in this project and I have forked the multibuild project to be able to use customized Manylinux containers.

You are correct about about build_wheels.sh and build_wheels_osx.sh. They can be removed and for example Qt and FFmpeg install commands on OS X as well as any exported env variables passed to CMake from them should be moved to setup.py. I have done one patch to OpenCV's CMakeLists.txt which disables i386 architecture (the build just didn't work without that hack back then) and there's also separate CMake toolchain file for macOS (probably can be removed).

It's better to do sdist and test as a separate tasks.

About Linux: afaik multibuild is usable only when the build runs in Travis or you have Docker installed on your machine. Docker is not needed for local builds when someone wants to build the project via sdist. This is why setup.py has to somehow know (maybe some env variable in .travis.yml) if the build is running on Travis or on someone's private machine to be able to decide correct build toolchain in case of Linux.

Copy link
Member

@skvark skvark left a comment

Choose a reason for hiding this comment

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

Looks good. Does the Travis machinery work like before after these changes? Would it be easier to do the Travis changes separately and make another PR after that?

'Operating System :: Microsoft :: Windows',
'Operating System :: POSIX',
'Operating System :: Unix',
'Programming Language :: Python',
Copy link
Member

Choose a reason for hiding this comment

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

These were added in a recent PR, could you add them in here also.

'Programming Language :: Python :: 2',
'Programming Language :: Python :: 2.7',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.4',
'Programming Language :: Python :: 3.5',
'Programming Language :: Python :: 3.6',

@native-api
Copy link
Contributor Author

native-api commented Dec 18, 2017

No, Travis doesn't work. I'm currently fixing it in https://github.com/native-api/opencv-python/tree/build_locally_travis and will merge into this branch here when I'm done. You can see results at https://travis-ci.org/native-api/opencv-python .

I think I'll make yet another config.sh (say, multibuild_customize.sh) to be sourced in before_install: to alter things without having to add yet another level of fork to multibuild :) . That should even let us use upstream multibuild -- it features updated Python download URLs and has been made more customizable since fork time.

@skvark
Copy link
Member

skvark commented Dec 18, 2017

Ok, then it's better to wait for the Travis changes. It's nice if we can use the upstream directly, forks bring always additional maintenance burden.

@native-api
Copy link
Contributor Author

Linux builds now work but I'm stuck with MacOS. Travis keeps building fat binaries (which times out the build) even though I specify CMAKE_OSX_ARCHITECTURES. I cannot reproduce this locally: in a Hackintosh VM, there's no -arch at all in compiler options. Ppl on the net suggest switching to CircleCI who provide SSH access. For now, I requested ssh access that Travis provides on request.

@skvark
Copy link
Member

skvark commented Jan 12, 2018

I had the same problem and that's why I patched OpenCV's CMakeLists.txt: https://github.com/skvark/opencv-python/blob/master/travis/disable_i386.patch

There's a 90 minute build timeout in my opencv-python Travis project which I have requested separately from Travis because the default 50 minutes is not enough even without fat binaries. I believe that if you merge the changes to this pull request the build does not timeout.

@native-api
Copy link
Contributor Author

native-api commented Jan 12, 2018

I found where these flags come from. It's multibuild, ... again!

@skvark
Copy link
Member

skvark commented Jan 12, 2018

Nice find! I gave up when I debugged the same issue last time.

@native-api
Copy link
Contributor Author

Okay, that should do it.

@skvark
Copy link
Member

skvark commented Jan 27, 2018

Sorry, haven't had time to look at this yet. I will check the code before merging if there are no new changes coming anymore.

@skvark skvark merged commit 86e87d7 into opencv:master Feb 19, 2018
@skvark
Copy link
Member

skvark commented Feb 19, 2018

Finally had time to read this through properly. Thank you very much. The code is now much cleaner and easier to manage.

@native-api native-api deleted the build_locally branch February 28, 2018 15:19
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.

3 participants