zshipko / halide-build

ISC License
8 stars 0 forks source link

halide path issues #52

Open ibtaylor opened 1 year ago

ibtaylor commented 1 year ago

I was having a hard time getting the test to run in halide-runtime.

I finally figured out that halide-build was looking for GenGen.cpp under <halide_path>/tools. It's now obvious to me that halide src (from halide-build) was expected to be run beforehand which results in $HOME/halide/tools existing. Having built halide from source myself and used cmake --install, I ended up with $HOME/halide/share/tools.

Using a symlink works around this issue for me, but it would probably be nice to get some kind of warning or error if some of these paths don't exist. It was not at all obvious that this was the issue from the build output.

Adding to fn build

let include = ...;
let tools = ...;
if !include.exists() {
    return io::Result::Err(io::Error::new(io::ErrorKind::NotFound, format!("Could not find {}", include.to_string_lossy())));
}
if !tools.exists() {
    return io::Result::Err(io::Error::new(io::ErrorKind::NotFound, format!("Could not find {}", tools.to_string_lossy())));
}

I ended up with a useful message when halide-runtime TEST=1 cargo test failed.

Thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Custom { kind: NotFound, error: "Could not find /home/user/halide/tools" }', build.rs:41:23

If you could give me any kind of direction on maybe some improvements that could be made on this front, I'd be willing to implement and issue a PR.

zshipko commented 1 year ago

Thanks for pointing this out! There is definitely a lot of room to improve the usability of this program. I don't think requiring the source to be managed using halide src is ideal, but that is the workflow I've been using.

It could be nice to make all the different paths configurable, so adding a flag like --tools-directory might be one fix. Another option, like in your example, would be to show which path is missing and to provide general directions to fix it using a symlink. A third option could be to search in both $HOME/halide/tools and $HOME/halide/share/tools - this could even be combined with option 1 or 2. What do you think?

ibtaylor commented 1 year ago

Halide is either going to be installed in a user directory or system-wide.

User directory could be how you did it via halide src. The way I did it, where you end up with include lib share via cmake --install. Or from an extracted bin release from the Halide project, where it is share/Halide/tools. This may be make distrib or something. This could also be different from make install.

System isn't great either. Ubuntu has stuff scattered out in different places https://packages.ubuntu.com/kinetic/amd64/libhalide14-0-dev/filelist I think ArchLinux was pretty reasonable /usr/include/Halide /usr/share/Halide/tools.

Seems like a messy situation.

I guess I can see something reasonably searching a couple of places relative to a <halide_path> and errors out if it doesn't find them. Each item could be overridden by environment variables as needed (preventing search). HALIDE_DIR HALIDE_TOOLS_DIR HALIDE_INCLUDE_DIR HALIDE_LIB_DIR or HALIDE_LIB (I don't know the details).

There is also the issue of static vs dynamic libs. I built Halide as static with Halide_BUNDLE_LLVM=ON. I don't know if things get more complicated with the dynamic library in needing to also link in a bunch of LLVM libs.

In any case, it'd probably be good to document the expected situation.

To be honest, I'm not a fan of the halide src automatic building thing. I don't really like programs pulling down software from repos and executing stuff. Also, the build is complicated enough that you really want to enable parallel compilation and probably configure targets and stuff depending on what you're doing.

zshipko commented 1 year ago

Yeah, I am not at all attached to halide src - it was helpful in the past, but should probably be removed altogether.

Your solution using the separate environment variables seems like a nice way to handle locating everything. As an on-and-off hobbyist user of Halide I'm not quite sure how most people are using it but my guess is that supporting the system installation by default makes the most sense and then adding the ability to customize the paths on top of that when using a non-system install.

In the case of handling extra dependencies like LLVM, I think providing a way to pass extra flags is the bare minimum - I wonder if it would be possible to leverage the installed cmake files to locate all the necessary resources?

ibtaylor commented 1 year ago

I did some investigating.

It doesn't seem like cmake --find-package is a viable option for a couple of reasons. I think the find_dependency(Threads) prevents this, but even after commenting it out: cmake --find-package -DCOMPILER_ID=Clang -DLANGUAGE=CXX -DNAME=Halide -DHalide_ROOT=$HOME/halide -DMODE=LINK provides no output. Maybe it only works with older-style FindXXX.cmake type modules.

Creating a tmp/CMakeLists.txt dummy and running cmake -S tmp -B build_tmp from this CMakeLists.txt...

cmake_minimum_required(VERSION 3.16)
project(test LANGUAGES CXX)
set(Halide_ROOT "$ENV{HOME}/halide")
find_package(Halide REQUIRED CONFIG)
message(STATUS "Halide_components: ${Halide_components}")
foreach(target
    Halide
    Generator
    LanguageOptions
    LLVM
    wabt
)
  foreach(property
      INTERFACE_LOCATION
      INTERFACE_COMPILE_FEATURES
      INTERFACE_INCLUDE_DIRECTORIES
      INTERFACE_LINK_LIBRARIES
      INTERFACE_SOURCES
  )
    get_target_property(TMP Halide::${target} ${property})
    if (TMP)
      message(STATUS "Halide::${target}_${property}: ${TMP}")
    endif()
  endforeach()
endforeach()

.. yields some outputs, but it looks like there would have to be further expansion and cleaning up of some of the fields. Also, not sure about the Halide_components stuff. There is probably a way to do this if you know exactly what we want out of it.