microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS
MIT License
23.19k stars 6.39k forks source link

[mlpack] mlpack installs mlpack.hpp header in the wrong place #40170

Closed rcurtin closed 3 months ago

rcurtin commented 3 months ago

Describe the bug

I noticed that after installing mlpack via vcpkg, the mlpack.hpp header is not in the right place. The installed directory structure should be:

install_dir/mlpack.hpp
install_dir/mlpack/core.hpp
install_dir/mlpack/base.hpp
install_dir/mlpack/prereqs.hpp
install_dir/mlpack/core/...
install_dir/mlpack/methods/...

and so forth.

However, the file mlpack.hpp is being installed as install_dir/mlpack/mlpack.hpp, which means that users cannot do

#include <mlpack.hpp>

as they should be able to.

I believe the problematic script install line is:

file(GLOB HEADERS "${SOURCE_PATH}/src/*.hpp"  "${SOURCE_PATH}/src/mlpack/*.hpp")

But this should be:

file(GLOB HEADERS "${SOURCE_PATH}/src/*.hpp"  "${SOURCE_PATH}/src/*.hpp")

Environment

To Reproduce Steps to reproduce the behavior:

  1. ./vcpkg install mlpack
  2. Look at directory structure of installed mlpack headers

Expected behavior mlpack.hpp should be in the root of the install directory.

MonicaLiu0311 commented 3 months ago

We hope to put all mlpack related header files in the mlpack/ folder to avoid conflicts with header files of other ports. The usage file is also designed according to this point, so this is not a mistake.

rcurtin commented 3 months ago

I think it is reasonable to say that there will not be another port with the file mlpack.hpp.

We intentionally distribute mlpack in the way that I described so that a user can use

#include <mlpack.hpp>

The way you are distributing it causes confusing issues for users, conflicts with mlpack's documentation, and breaks most CMake scripts that are intended to find mlpack. Users of vcpkg+mlpack are forced to use one of the following workarounds---but again only when using vcpkg:

  1. If using Windows+vcpkg only, a user would have to do #include <mlpack/mlpack.hpp> which goes against every piece of documentation that mlpack provides, and goes against every tutorial that mlpack provides;

  2. If creating a project using mlpack that can be built in a vcpkg environment and also elsewhere, the include process looks like

    #if defined(HAS_MLPACK_VCPKG) // you would have to define this macro to detect the situation
    #include <mlpack/mlpack.hpp>
    #else
    #include <mlpack.hpp> // like all of the mlpack documentation
    #endif
  3. In a build system, if using scripts like Findmlpack.cmake, this script will return the incorrect MLPACK_INCLUDE_DIR and so you would have to detect when mlpack.hpp is in the wrong place and add an additional include directory to catch that situation.

  4. If using Visual Studio, the include directories will need to be changed to also include the mlpack/ directory and the parent directory, since other mlpack code may directly do includes of the form #include <mlpack/core.hpp> or similar.

I very much appreciate the work that has gone into packaging mlpack for vcpkg; this has made it much easier for users to develop mlpack on Windows. I regularly point users towards vcpkg for this reason. But not all users are sophisticated enough to see in the usage file that the headers are in different places (and at least to me, the message in that file is not really clear, unless I happened to already know that structure), so this is a point of confusion and friction.

We built mlpack's directory structure to match Armadillo's, and ensmallen's, and numerous other C++ header-only libraries where the main header is in the main include directory. In fact, it appears that the Armadillo and ensmallen ports do not have special handling of their <armadillo> and <ensmallen.hpp> headers; if it is not a problem there, can we not do the same thing for mlpack?

rcurtin commented 3 months ago

Thanks so much, appreciate the fixes! :+1: (and sorry for the very long explanation)