halide #2559
halide #2559
Conversation
need to avoid compiling with env clang++ on macOS
needs a Makefile patch to respect LDFLAGS
|
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 ( |
recipes/halide/meta.yaml
Outdated
| - toolchain | ||
| - clangdev | ||
| - llvmdev | ||
| - zlib # [linux] |
There was a problem hiding this comment.
I added it because the build was failing on Linux without it, but OK on Mac. Now that I think about it, this should be a build and run dep on both. Fixed now.
recipes/halide/meta.yaml
Outdated
| test: | ||
| requires: | ||
| - toolchain | ||
| - zlib # [linux] |
There was a problem hiding this comment.
Same as above. Why not Mac also?
recipes/halide/meta.yaml
Outdated
| simplejson is a simple, fast, complete, correct and extensible | ||
| JSON <http://json.org> encoder and decoder for Python 2.5+ and | ||
| Python 3.3+. It is pure Python code with no dependencies, but includes | ||
| an optional C extension for a serious speed boost. |
There was a problem hiding this comment.
This needs to be updated or removed. Seems to be the same as the example.
| build: | ||
| - toolchain | ||
| - clangdev | ||
| - llvmdev |
There was a problem hiding this comment.
Will we need some run time library as a run dependency then?
There was a problem hiding this comment.
Guessing not as we don't use these to compile the code. Though I'm a little fuzzy on what they are being used for currently.
There was a problem hiding this comment.
clang is used to parse some things at build time, and llvm gets statically linked (effectively), so these are not runtime dependencies. There are no runtime dependecies for the static lib, and only zlib for the dylib.
There was a problem hiding this comment.
Right, forgot about the static libraries in llvmdev/clangdev.
| @@ -0,0 +1,10 @@ | |||
| if [[ $(uname) == "Darwin" ]]; then | |||
| # don't use clang++ from the env | |||
| export CXX=c++ | |||
There was a problem hiding this comment.
Guessing this shouldn't be needed here.
There was a problem hiding this comment.
toolchain sets CXX=clang++, which normally resolves to /usr/bin/clang++. When clangdev is installed in conda, the version in the env will be used instead of the version toolchain normally expects. This doesn't work (I haven't investigated why, but the conda-packaged clang can't build the library). An equivalent fix would be setting it to /usr/bin/clang++.
If toolchain set export CXX=/usr/bin/clang++ so that it always pointed to the same compiler regardless of what's in the env, this wouldn't be necessary.
There was a problem hiding this comment.
It's a good point. Should we open a PR at the toolchain and discuss? 😉
There was a problem hiding this comment.
| @@ -0,0 +1,16 @@ | |||
| if [[ $(uname) == "Darwin" ]]; then | |||
| # don't use clang++ from the env | |||
| export CXX=c++ | |||
There was a problem hiding this comment.
Interesting. Maybe we should discuss the pros and cons of this choice as it pertains to Halide.
| generator_test(); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
nit: A terminal newline would be nice here.
|
Very excited to see Halide being added to conda-forge. Not super familiar with the build process for Halide so may be asking some naive questions above. Mainly they focus on how and with what we build Halide and what that means for the package and how it can be used. |
| test: | ||
| requires: | ||
| - toolchain | ||
| - zlib |
There was a problem hiding this comment.
Guessing we can drop this, zlib, as it is already a run time dependency.
|
There are some minor changes, but they can be saved for the feedstock. Especially as this is passing on all CIs. 🎉 |
|
Thanks @minrk. |
ensures that enabling toolchain specifies a specific compiler on the system, and not the one that may be installed in the env with the same name, unless explicitly requested. This is the right thing to do for halide (conda-forge/staged-recipes#2559 (comment)), but may not be what people want in general. When people install gcc as a build dep (and toolchain), do they expect the one in the env to be used by default without being explicitly selected? Should they?
cc @jrk
cc @arokem who opened #752
closes #752