-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
VTK 7 (py2 and py3) #453
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
VTK 7 (py2 and py3) #453
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/vtk:
|
Hi @Korijn! Thanks for submitting!
Awesome. Glad conda-forge can help!
👍 The CI looks a bit scary when it is all red like that, but hopefully we can iterate on it. To break down the problems by OS: Linux:
I'm afraid I know very little about X11, but I notice that libxt isn't part of the manylinux spec. For now, you can add a He is currently away, but @jakirkham may be able to help in deciding whether we need to package libxt within conda-forge, or whether we simply make it a requirement for using the conda-forge distribution. OSX:
Windows. Took 43min on Py27, 1 hr (and timed out) on py34 x86. The timeouts are problematic - we have an hour to build on item of the matrix, and unfortunately that is a hard limit put onto us from AppVeyor. I don't know that there is anything we can do about that in terms of either a) what we build or b) asking for longer. The win x64 (py27):
Hope the failures aren't demoralizing - this is a really cool recipe to package and I'm super pleased you've submitted a PR to fix the current state of VTK on conda.` |
I can address most of these problems. About the x64 2.7 build; do you have a custom identifier for the c compiler? This recipe is tested and working on a Windows 7 VM with VS2008 Pro SP1 which has the x64 compiler built-in. |
I think the tricky part is that we have AppVeyor here. It uses VS 2008 On Mon, Apr 25, 2016 at 5:30 PM Korijn van Golen [email protected]
|
I can't make sense of it right away. https://ci.appveyor.com/project/conda-forge/xonsh-feedstock/build/1.0.14/job/827pntj23q0xxt0p#L202 seems to be one issue - Windows 7.1 SDK not having the 64-bit compiler, possibly. I'm pretty sure @patricksnape tested that, though. The other issue appears to be the same: https://ci.appveyor.com/project/conda-forge/staged-recipes/build/1.0.1375/job/jq2rrc7uwjb8iehw#L213 This should be a quick fix once we figure out what the correct value should be. I can help push a quick release. PS: @patricksnape it is better to set variables like:
rather than https://ci.appveyor.com/project/conda-forge/xonsh-feedstock/build/1.0.14/job/827pntj23q0xxt0p#L198 Both treat spaces well, but the former doesn't have quotes in the end variable value, and is easier to deal with. Sorry I didn't catch this on the PR. |
Might need |
To help preserve our sanity, I moved the weird Windows build issue into this issue ( conda/conda-build#895 ). IMHO we have at least confirmed there is a conda-build bug at this point. Maybe we can focus the discussion there. Tracking it over multiple PRs, issues, and feedstocks is getting a little out of hand. |
Also, I'm wrong about needing that workaround. I forgot the new issue is restricted to Windows 64-bit Python 3.4. So, we shouldn't need the old VS 2008 64-bit workaround as the downgrade of |
Ignore the above. Also, don't dig any deeper. I know the cause. |
So, this PR ( #455 ) should fix things for you here at staged-recipes. Want to give it a try? |
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Changes:
I'm not too sure what has to be done for OS X; I have no machine to test on. |
cmake --build . --target INSTALL --config %BUILD_CONFIG% | ||
if errorlevel 1 exit 1 | ||
|
||
exit /b 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a terminal newline.
Is it possible to disable X11 support on Mac? If so, that's what I would do here. |
I'm clueless about X11 and also about Mac. There's probably a cmake flag to disable it. |
There is. Just set this on Mac only |
The thing with Mac is they used to have X11 support baked in, but they decided to drop it. Though one can still install it and Apple still supports this open source effort. It is just something we can't expect to find on the user's system. Maybe one day we can add a package to install it in a conda environment. Until then, disabling is the right way to go. |
So, I don't think he mentioned it, but @msarahan had a recipe for this that was in the process of being added. Not to speak for him, but you seem motivated to get this in and I think we are happy to see someone pushing this forward so please continue (plus I'm sure @msarahan has tons on his plate and doesn't mind someone taking this off it 😉). Though you might be able to pick up a few useful things from his recipe, which can be found in this PR ( #285 ). |
@stuarteberg, given your use of VTK, maybe you have some useful feedback here. Please feel free to share. |
-DPYTHON_LIBRARY:FILEPATH=$library_file_path | ||
fi | ||
|
||
make -j${CPU_COUNT} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add ctest
. Also, you may need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been a little cautious about using CPU_COUNT
as the CIs don't always report this correctly. For instance, they may report the number of CPUs for the node you are on instead of noting how many of these you get. Could we try removing this? If the build time is too long, we can play with this more.
I don't have much to offer. I use a pretty old version of VTK. Recipe is here: One possible suggestion: Our recipe sets the |
REM tell cmake where Python is | ||
set PYTHON_LIBRARY=%PREFIX%\libs\python%PY_VER:~0,1%%PY_VER:~2,1%.lib | ||
|
||
cmake .. -G"%GENERATOR_NAME%" ^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use NMake here too, if it fails on 2.7
We've rebased again, processed the comments, and cleaned up using the things we learned from the |
@thewtex or @dlonie Would any of you guys at Kitware mind having a look at our build scripts to see what we can do to get our VTK recipe working? |
The good news:
The bad news:
I'm currently working under a tight schedule and need to get at least the Linux packages out there. I would like to propose the following:
We should then get green light on the CI and be able to merge. We can then open a new PR on the feedstock where we'll work on:
@jakirkham, thoughts? |
@Korijn that is a sound strategy (I did something similar with 👍 |
@ocefpaf cool. I commited the proposed changes, let's see if CI turns all-green. |
Fingers crossed! |
-DCMAKE_BUILD_TYPE=$BUILD_CONFIG \ | ||
-DCMAKE_INSTALL_PREFIX:PATH="${PREFIX}" \ | ||
-DCMAKE_INSTALL_RPATH:PATH="${PREFIX}/lib" \ | ||
-DINSTALL_PKGCONFIG_DIR:PATH=$PKG_CONFIG_PATH \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INSTALL_PKGCONFIG_DIR
is unused according to a CMake warning. will remove that in future PR.
@ocefpaf can we merge? I'll open a PR on the feedstock directly afterwards. |
Thanks @Korijn! I know the pain to fight against the CI limitations. Thanks for leading this effort! |
We had the same rough experience adding OpenCV, @Korijn. This sounds totally sensible to me. I'm glad this was added as is. Also, good job getting this to build. 🎉 |
This work is continued here: |
We built VTK 7 successfully using this recipe for the full range of Python versions (2.6, 2.7, 3.3, 3.4, 3.5) on x64 windows and linux architectures, so I reckon it should work on x86 and OS X as well.
The main reason for writing it is that the VTK package in the official anaconda channels is lagging behind and doesn't support python 3.x.
Looking forward to improving this recipe together with you all.