[HIPIFY] Binary not installed into "bin" dir

[Flamefire]

The documentation claims

The binary can then be found at ./dist/bin/hipify-clang.

However this is not true because the "bin" folder is not passed at https://github.com/ROCm-Developer-Tools/HIPIFY/blob/397b58a8fc38fa9c5c3252ccfa6e9cd1f0f141a5/CMakeLists.txt#L105

it seems you could pass -DBIN_INSTALL_DIR=bin to get that due to https://github.com/ROCm-Developer-Tools/HIPIFY/blob/397b58a8fc38fa9c5c3252ccfa6e9cd1f0f141a5/CMakeLists.txt#L102 but then everything will be installed into that folder, not only the binary, see https://github.com/ROCm-Developer-Tools/HIPIFY/blob/397b58a8fc38fa9c5c3252ccfa6e9cd1f0f141a5/CMakeLists.txt#L109

See also https://github.com/ROCm-Developer-Tools/HIPIFY/issues/79 which is basically the same issue and not fixed, at least when building HIPIFY standalone.

Also the https://github.com/ROCm-Developer-Tools/HIP/pull/1593 does not really fix this as it will still make everything go into bin

Side note: Using relative paths in the install commands should be preferred. They are automatically made absolute by prepending CMAKE_INSTALL_PREFIX. I'd also suggest to use https://cmake.org/cmake/help/v3.13/module/GNUInstallDirs.html to set defaults for DESTINATION, e.g. CMAKE_INSTALL_BINDIR

[emankov]

I'd also suggest to use https://cmake.org/cmake/help/v3.13/module/GNUInstallDirs.html to set defaults for DESTINATION, e.g. CMAKE_INSTALL_BINDIR

CMAKE_INSTALL_BINDIR appeared with cmake 3.14: https://cmake.org/cmake/help/v3.14/command/install.html?highlight=cmake_install_bindir. hipify-clang's cmake_minimum_required is 3.8.0. Moreover, hipify-clang installation has a simple and plain folder structure, which has no RUNTIME, LIBRARY, or HEADER subdirectories distribution. This plain organisation is already used in hipify tools packages, so there is no need in CMAKE_INSTALL_BINDIR introduction in CMakeLists.txt.

[emankov]

Fixed by #463.

[Flamefire]

CMAKE_INSTALL_BINDIR appeared with cmake 3.14: https://cmake.org/cmake/help/v3.14/command/install.html?highlight=cmake_install_bindir. hipify-clang's cmake_minimum_required is 3.8.0

Just FYI: It was there for a while and also exists in 3.8. See the docs for that module: https://cmake.org/cmake/help/v3.8/module/GNUInstallDirs.html

And for #463: Instead of FORCE setting a cache variable you can simply set a regular variable which IIRC just shadows the cache variable. And as you always force-set it, maybe make it an error to have it set or get rid of it and simply use the standard CMAKE_INSTALL_PREFIX, see e.g. https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT.html

[emankov]

Just FYI: It was there for a while and also exists in 3.8. See the docs for that module: https://cmake.org/cmake/help/v3.8/module/GNUInstallDirs.html

Nope, CMAKE_INSTALL_BINDIR is not presented in 3.8, it appears in cmake 3.14; please, double-check. Anyway, CMAKE_INSTALL_BINDIR is unnecessary here.

or get rid of it and simply use the standard CMAKE_INSTALL_PREFIX, see e.g.

Do you want the following:

if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
    set(CMAKE_INSTALL_PREFIX "${PROJECT_BINARY_DIR}/dist" CACHE PATH ${INSTALL_PATH_DOC_STRING} FORCE)
endif()

install(TARGETS hipify-clang DESTINATION ${CMAKE_INSTALL_PREFIX})

[Flamefire]

Nope, CMAKE_INSTALL_BINDIR is not presented in 3.8, [...]

The above linked doc is from 3.8 and mentions CMAKE_INSTALL_BINDIR (CMAKE_INSTALL_<dir> [...] where <dir> is one of: BINDIR [...])

Do you want the following:

Exactly. This is how CMake itself documents that use case:

This can be used by project code to change the default without overriding a user-provided value:

https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT.html#variable:CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT

[emankov]

CMakeLists.txt was simplified more with #465.

The above linked doc is from 3.8 and mentions CMAKE_INSTALL_BINDIR (CMAKEINSTALL

[...] where is one of: BINDIR [...])

Sorry, you are right.