[HIPIFY][fix] cmake: NO_DEFAULT_PATH is strongly needed in find_package for LLVM

[emankov]

Otherwise LLVM will be searched in system folders.

[ChrisKitching]

What's wrong with finding llvm in system folders? Plenty of distros (most of them!) ship an LLVM that's compatible with hipify now, so this should work fine. Why require users to go to the extra hassle of manually unpacking llvm somewhere?

That made sense when hipify only worked with a specific version of llvm. But now I think it's just unhelpful..?

[emankov]

What's wrong with finding llvm in system folders?

Well, actually we've got a mess here:

find_package(LLVM PATHS ${HIPIFY_CLANG_LLVM_DIR} REQUIRED)

We specify an exact path to LLVM to be used, but actually the found LLVM might be different.

Why require users to go to the extra hassle of manually unpacking llvm somewhere?

It was so from a very beginning: we were supporting only LLVM 3.8 and later 3.9. Now with your contribution in supporting of newest LLVM versions, we can think about eliminating LLVM specifying requirement, but not right now. Let's merge all the rest of your pull requests and ensure that all is build and work as it was before (or even, I believe, better), ok? :-).

That made sense when hipify only worked with a specific version of llvm. But now I think it's just unhelpful..?

Right, answered above :-)

[ChrisKitching]

We specify an exact path to LLVM to be used, but actually the found LLVM might be different.

The PATHS argument to find_package adds additional paths to be searched, it doesn't make find_package only look there.

I agree that having a PATHS argument at all is problematic. CMake already has standard mechanisms for users to tell it where to look for libraries they have put in unusual locations: have a look at the documentation I linked - it describes the search procedure in nauseating detail about halfway down. It's useful to trawl through it at least once: I recommend whiskey.

The concept is that find_package is used by projects to describe what they want, "I want version X of library Y!". CMake then provides a sane platform-specific search procedure, and the user can use standard CMake variables such as CMAKE_PREFIX_PATH to tell CMake where to find libraries they might have put in strange locations. Arguments like PATHS and HINTS are, except for in unusual circumstances, primarily intended for authors of finder-packages.

For this reason, having a project-specific "exact location of LLVM" variable is flawed. If you really want to specify a specific LLVM for it to use, you can just prepend its path to CMAKE_PREFIX_PATH on the command-line, and users don't have to learn about the special magic variable.

It was so from a very beginning: we were supporting only LLVM 3.8 and later 3.9. Now with your contribution in supporting of newest LLVM versions, we can think about eliminating LLVM specifying requirement, but not right now. Let's merge all the rest of your pull requests and ensure that all is build and work as it was before (or even, I believe, better), ok? :-).

The note of caution certainly has merit, especially given that everything caught fire yesterday. 😉

However, the proposed change has no effect on which version of LLVM it looks for, it just restricts where it looks. If you want to force it to keep using version 3.8 or 3.9, add a version argument to the find_package call instead.

Note, however, that such version pinning comes with some downsides:

• Compilation with 3.x is roughly 20x slower than with 5.0. This was actually my original impetus for doing the earlier work: it's a huge pain to work on hipify when it takes 20 minutes to recompile and test!
• Clearly there are people already benefiting from support for newer versions. That Matt dude whose computer caught fire last night is apparently using 4.0, and my distro ships 5.0. I've been unpacking old versions to test compatibility, but I'd rather not be forced into using the older version for ongoing development.

In general, I think what we want to do is:

• No HIPIFY_CLANG_LLVM_DIR variable at all: use cmake's standard mechanisms for this.
• option(BUILD_HIPIFY_CLANG "Build the hipify-clang utility" OFF) in the top-level CMakeLists.txt, allowing users to turn hipify on/off as they like. This has a number of advantages over the current approach, including documentation, visibility in cmake-gui, and no strange dependency on the HIPIFY_CLANG_LLVM_DIR variable for feature-toggling. See option().
• Set up CI to manipulate CMAKE_PREFIX_PATH to test compilation with all versions of LLVM since 3.8. I don't know what the status of CI is on this project, but it looks like you've got a Jenkinsfile so I have hope 😁

Sorry for the long post. Hopefully I explained myself better now... If you really want to do it this way, I'll keep a local reversion for my work and hopefully we can come up with something neater later on.

[emankov]

Well... Will try to be laconic.

However, the change you've proposed has no effect on which version of LLVM it looks for, you're just restricting where it looks.

Right, it only demands to search LLVM only in the path provided.

In general, I think what we want to do is: ...

I think more or less the same, but ... let it be the next small incremental steps.