Iiboost recipe#2931
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/iiboost:
|
|
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 ( |
|
Possibly of some interest to you, @stuarteberg. |
| build: | ||
| - python | ||
| - toolchain | ||
| - libgcc # [linux] |
There was a problem hiding this comment.
I replaced libgcc with gcc in the build requirements and left libgcc in the run requirements. Is this ok?
| imports: | ||
| - iiboost | ||
| commands: | ||
| - python -c "import iiboost" |
There was a problem hiding this comment.
This command and the one on Line 43 does the same thing
| - itk 4.11.* | ||
| - boost 1.63.* | ||
| run: | ||
| - libgcc # [linux] |
There was a problem hiding this comment.
This one might be needed if they are using OpenMP as stock CentOS 6 doesn't include that library.
There was a problem hiding this comment.
Ah, okay. Is gcc needed as a build requirement?
There was a problem hiding this comment.
Yeah, that's probably the best way to go.
|
FWIW as we are not building on Windows, can just add |
| -DIIBOOST_USE_OPENMP=$ENABLE_OPENMP \ | ||
| .. | ||
|
|
||
| make |
There was a problem hiding this comment.
Is there a make check or similar we can run?
There was a problem hiding this comment.
There's a python test that can be run. I enabled it on the py2 builds only, as it uses pickled data which cannot be unpickled on py3.
There was a problem hiding this comment.
It also segfaults on osx, so I disable it there as well.
https://travis-ci.org/conda-forge/staged-recipes/builds/229909874
There was a problem hiding this comment.
I think setting pickle.DEFAULT_PROTOCOL to 0 should fix this issue on Python 3 and be a no-op for Python 2 as 0 appears to be the default on Python 2. Might be worth a try.
There was a problem hiding this comment.
@jakirkham Unfortunately it does not seem to help, I am getting the same error message.
There was a problem hiding this comment.
I just tried to set the default protocol to zero on top of the test files. Is there something else I should've done? Perhaps some conversion tool? (sorry I am not that well versed with the pickle intricacies)
There was a problem hiding this comment.
It might need to be set wherever pickle is imported. Initially I was looking for an environment variable for this, but didn't find one. Maybe this is more useful. Would raise this point for discussion in the Python 3 PR.
There was a problem hiding this comment.
I guess it is using joblib to do this. FWICT scikit-learn developers ran into the same problem and solved it by having separate files for Python 2 and 3.
|
|
||
| about: | ||
| home: https://github.com/cbecker/iiboost | ||
| license: GPL3 |
There was a problem hiding this comment.
Please add license_file: LICENSE.txt.
|
|
||
| build: | ||
| number: 0 | ||
| skip: True # [win] |
There was a problem hiding this comment.
Seems the CIs are running out of time with the different python and numpy requirements. Would recommend skipping NumPy 1.11 for now by changing win -> win or np!=112. Can remove this in the feedstock and re-render to get the older NumPy version into the build matrix.
[skip appveyor]
[skip appveyor]
[skip appveyor]
[skip appveyor]
[skip appveyor]
This reverts commit 24d59bd.
|
@isuruf running into issues with LF/CLRF conversion. There's a file in the source tree which uses Windows style line endings, and when I create a patch from it, it gets applied successfully on Windows but not on linux (because of the git automatic line-ending conversions I guess?). Any idea on how to proceed? |
This reverts commit 3855fdb.
|
Use the patch only on windows? |
|
I'll try that next :) Thanks. |
|
I have removed the windows build for the moment, as there seems to be a cmake issue in ITK. I have reported it here: https://issues.itk.org/jira/browse/ITK-3545 Basically, it seems like the CMake support for the hdf5 submodule is broken on Windows, as it will try to link to the hdf5 libs with the wrong prefix (actually, no prefix at all). I'll wait a while to see what happens with the report, otherwise a possible fix is to build the internal version of hdf5 on Windows I guess? pinging @blowekamp as well. |
This reverts commit 55bb5c0.
|
Thanks @blowekamp, seems to be working now. |
| - numpy x.x | ||
| - itk 4.11.* | ||
| - boost 1.63.* | ||
| - scikit-learn |
There was a problem hiding this comment.
Is this only a build requirement?
There was a problem hiding this comment.
It is a build requirement in the sense that the tests run as part of the build script require scikit-learn.
There was a problem hiding this comment.
Tests should be run in the test section, not in the build script :)
There was a problem hiding this comment.
This raises a question that (I think) there isn't any good answer to. Many C++ libraries include tests that can be executed via make test or make check, etc. But those tests assume the binary hasn't been installed yet. They test it in-place. Conda-build's test step happens too late to run those checks. What is considered "best practice" in such cases? For now, I think simply running the tests as part of the build step is reasonable, but I'm open to suggestions.
Side note: It would be nice of conda-build exposed an environment variable (say, NO_TEST) within build.sh to indicate whether or not the user is building the recipe with the --no-test flag...
There was a problem hiding this comment.
Yes, that is true @stuarteberg - I even maintain a bunch like that. But this package isn't in that situation, is my understanding :)
| patches: | ||
| - cmath_fixes.diff | ||
| - python3.diff | ||
| - msvc_fixes.diff |
There was a problem hiding this comment.
Please submit these patches upstream.
Also patching in Python 3 support for Python 2 only libraries has been frowned upon by other reviewers and core members in the past. Though I get that being stuck on an old Python because of a dependency is a bit of a drag especially for a few lines of code. Though I think we will have a good argument for doing this if upstream adds Python 3 support into master (potentially using a variant of this patch). Let's see what they say.
There was a problem hiding this comment.
Some of the MSVC issues are solved in master already. I opened a PR for the remaining ones here:
There was a problem hiding this comment.
And here's the python3 PR:
| - eigen | ||
| - numpy x.x | ||
| - itk 4.11.* | ||
| - boost 1.63.* |
There was a problem hiding this comment.
There seem to be a few other build and run requirements not included in this list. Looks like they may be transient requirements. For example, I'm seeing zlib, expat, hdf5, jpeg, libpng, gdcm just by looking at the linkages fixed on the Travis CI build. Would recommend adding these and pinning them. Also for any that don't have a pinning, it would be good to look into a reasonable pinning. We often turn to ABI Laboratory for this sort of thing. Here is the history on GDCM for example.
Note: This appears to be linking to the C++ interface of HDF5. ( cc @stuarteberg )
There was a problem hiding this comment.
These sounds mostly like ITK dependencies. Having each project which use another project maintain the same list of build ( and runtime? ) dependencies seems rather problematic, error prone, burdensome and bound for failure.
There was a problem hiding this comment.
@jakirkham I thought these are transitive dependencies from ITK?
There was a problem hiding this comment.
As noted above, these are being linked to by iiboost libraries directly. Hence why install_name_tool is fixing these linkages in iiboost's libraries. Feel free to peruse the log if you are not convinced. The thing is if we don't list them here, it is going to be more error prone than if we do.
For example, say the jpeg version that itk uses changes. Will that break iiboost? FWICT based on these linkages yes. So we will need to rebuild iiboost. However if we haven't been tracking jpeg in iiboost previously, and we merely bump the build number on iiboost, how can we be sure which package of itk, iiboost, and jpeg we get during an install. If we get the old itk package pinned to the old jpeg and a newer iiboost package, then iiboost will be broken. Similarly if we have the newer itk and jpeg, but an older iiboost. The only way to safeguard against these problems is to pin iiboost dependencies within its own recipe.
There was a problem hiding this comment.
Yes, iiboost is directly being linked against these libraries even though it does not appear to directly need them. This is because it it using and linking against all ITK libraries in CMake:
https://github.com/cbecker/iiboost/blob/master/CMakeLists.txt#L100
It would be better practice to only specify the required ITK COMPONENTS (modules and IO) in the find_package(ITK). This should make these libraries be only "transitive" dependencies.
There was a problem hiding this comment.
That's really a discussion to have with upstream. Given we are just packaging this library, the best we can do is try to make sure things don't break when it is installed. Hence my recommendation of listing all of these dependencies explicitly.
There was a problem hiding this comment.
Another option is to do something like, bluescarni#1
There was a problem hiding this comment.
Here's a log, https://travis-ci.org/isuruf/staged-recipes/builds/234882892
Note that only the ITK libs are linked here.
| - python | ||
| - numpy x.x | ||
| - itk 4.11.* | ||
| - boost 1.63.* |
There was a problem hiding this comment.
If you want, you can drop boost here, because it doesn't seem to be linking to it.
There was a problem hiding this comment.
These pinnings may need to be updated
[skip appveyor] Use dylibs as-needed
|
Hi friend! We really, really, really appreciate that you have taken the time to make a PR on In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on master so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the Cheers and thank you for contributing to this community effort! |
|
Hi again! About a month ago, we commented on this PR saying it would be closed in another month if it was still inactive. It has been a month and so now it is being closed. Thank you so much for making it in the first place and contributing to the community project that is Cheers and have a great day! |
7 participants
No description provided.