[popsift] add new port v0.9

[simogasp]

Describe the pull request
It add a new port for popsift library for sift feature detection in images.
It depends on Cuda and optionally on boost if the tools are built.

• Which triplets are supported/not supported? Have you updated the CI baseline?
  It builds on win, osx, and Linux, shared or static

• Does your PR follow the maintainer guide?
  yes

[msftclas]

All CLA requirements met.

[JackBoosY]

When building x86-windows:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
CUDA_cublas_LIBRARY (ADVANCED)
    linked by target "popsift" in directory C:/agent/_work/1/s/buildtrees/popsift/src/v1.0.0-rc1-ce675fc178/src

-- Configuring incomplete, errors occurred!

When building x64-windows-static:

Expected Debug,Static crt linkage, but the following libs had invalid crt linkage:

    C:/agent/_work/1/s/packages/popsift_x64-windows-static/debug/lib/popsiftd.lib: Release,Static

[fabiencastan]

I can reproduce the build error on "x64-windows-static" but I do not understand the problem. Does it mean that the library is built in Release instead of Debug?

[JackBoosY]

@fabiencastan Yes, that's right.

[JackBoosY]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JackBoosY]

When building popsiftx64-linux:

CMake Error at /mnt/1/s/downloads/tools/cmake-3.14.0-linux/cmake-3.14.0-Linux-x86_64/share/cmake-3.14/Modules/FindCUDA.cmake:700 (message):
  Specify CUDA_TOOLKIT_ROOT_DIR
Call Stack (most recent call first):
  /mnt/1/s/scripts/buildsystems/vcpkg.cmake:329 (_find_package)
  CMakeLists.txt:89 (find_package)

[simogasp]

@JackBoosY thanks for the log
I don't understand though. Normally if we declare the dependency on cuda it would be up to the cuda port to declare and set CUDA_TOOLKIT_ROOT_DIR, as it already checks for the cuda install.
And I don't see why it works on windows and not on Linux: is cuda installed in some non-standard location on the azure machine?
In a recent change of the cuda port it only searches for system directories such as /usr/local/cuda-*
https://github.com/microsoft/vcpkg/blob/master/ports/cuda/portfile.cmake#L20
Could be that the issue?

[simogasp]

BTW, it works ok on my local machine and I don't have any special env variable set for cuda

[simogasp]

@JackBoosY I'm sorry but I'm still struggling trying to figure out this Linux problem.
We have a simple line in our code to find cuda
find_package(CUDA 7.0 REQUIRED)
and indeed it is the findCUDA.cmake module that fails:

/mnt/1/s/downloads/tools/cmake-3.14.0-linux/cmake-3.14.0-Linux-x86_64/share/cmake-3.14/Modules/FindCUDA.cmake:700 (message):

which can be found here https://github.com/Kitware/CMake/blob/v3.14.0/Modules/FindCUDA.cmake#L700

FindCUDA is normally able to find cuda in the most common installation places, if it is installed in some weird places, you can pass CUDA_TOOLKIT_ROOT_DIR to tell him where to look.

Even if I wanted to add this to our portfile I still have no clue on what to set it to:

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS ${FEATURE_OPTIONS} -DCUDA_TOOLKIT_ROOT_DIR:PATH="???????"
)

Is there any variable that has this information? Whenever the cuda portfile is called, are the variables set there visible by other packages? (I hope not as this can lead to a clusterf**k of name conflicts...)

[JackBoosY]

@simogasp Yes, I‘m investigating this issue in depth.
The port cuda can already find the libraries and tools correctly, but cmake cannot.

[JackBoosY]

Hmm... I have no permission to push to your branch.
Here is my changes: popsift.zip

[simogasp]

thanks @JackBoosY for the patch, I'm pretty sure that with this it is gonna work.

On the other hand, I'm not sure that is the correct solution to put in place because we are just replicating the code of cuda portfile, which means we have to maintain it twice or more if the same patch it is applied to all the other packages depending on Cuda.

It seems that our package is the only one, so far, that has a mandatory dependency on Cuda, for all the others (darkned, opencv etc) it is only optional: I imagine that the CI does not build the optional features otherwise they would have the same issue as well.

I think that at this point we should figure out a way to export this CUDA_TOOLKIT_ROOT_DIR variable.
I'll try to spitball some ideas:

1. Obviously we can rely on the user to set it up as env variable because it is often what developers do and for sure if it exists we can rely on that to find Cuda (I'm talking about the Cuda portfile). On the other hand, we cannot make this mandatory because not everybody is familiar with low-level developing stuff such as setting env variables.

2. At the moment the Cuda portfile does nothing other than checking Cuda is there and it is the right version. The only thing that it installs is the abi txt with the various info. What could be done is for the portfile to export a simple CMake file, say cuda.cmake (that will be installed in share/cuda) where the CUDA_TOOLKIT_ROOT_DIR is set to the proper value.
   The port that depends on Cuda then will include cuda.cmake to get the value of CUDA_TOOLKIT_ROOT_DIR and pass it at CMake config.
   This requires to access directly to the install folder of vcpkg and I don't know if it goes against the principles and the philosophy of the package manager.
   But I think it is more maintainable solution than having each package personally looking for Cuda.
   Each package will have to do something like:

if(NOT DEFINED CUDA_TOOLKIT_ROOT_DIR)
  include(${VCPKG_INSTALL}share/cuda/cuda) # this is wrong but I don't know if there is a variable that we can use that tells the install path
endif()
...
vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS ${FEATURE_OPTIONS}
    -DCUDA_TOOLKIT_ROOT_DIR:PATH=${CUDA_TOOLKIT_ROOT_DIR}
...
)

I think that something like that will benefit all the other packages that depend on Cuda.
Of course, it may not be the right or best solution, I don't know all the details of vcpkg to a have a better insight.
Since the problem is related to Cuda rather than our package we can also open a dedicate issue to discuss about that and have feedbacks from other maintainers.

thanks again for your help!

[JackBoosY]

Yes, that's only a workaround.

According to FindCUDA.cmake, has the cuda default path been changed?

@strega-nil What do you think about?

[BillyONeal]

I observe that the port is trying to do find_package(CUDA. FindCuda.cmake says:

  Superseded by first-class support for the CUDA language in CMake.

Replacement
^^^^^^^^^^^

It is no longer necessary to use this module or call ``find_package(CUDA)``.
Instead, list ``CUDA`` among the languages named in the top-level
call to the :command:`project` command, or call the
:command:`enable_language` command with ``CUDA``.
Then one can add CUDA (``.cu``) sources to programs directly
in calls to :command:`add_library` and :command:`add_executable`.

The port is looking for CUDA 7 but our CI machines have CUDA 10.1 (soon to be 10.2). Perhaps the deprecated script hasn't been updated for the current versions?

We install CUDA here:

vcpkg/scripts/azure-pipelines/linux/provision-image.sh

Line 38 in 5a80a8e

sudo apt install -y --no-install-recommends cuda-compiler-10-2 cuda-libraries-dev-10-2 cuda-driver-dev-10-2 cuda-cudart-dev-10-2 libcublas10 cuda-curand-dev-10-2

[BillyONeal]

I see that other ports that want CUDA have set(ENV{CUDACXX} "$ENV{CUDA_PATH}/bin/nvcc") in their portfile.cmake, that might be necessary.

[JackBoosY]

According to the official documentation:

Deprecated since version 3.10: Superseded by first-class support for the CUDA language in CMake. Superseded by the FindCUDAToolkit for CUDA toolkit libraries.

So, can you report this to upstream?

Thanks.

[BillyONeal]

It looks like we need to come up with some way of exporting the path to CUDA determined by our cuda\portfile.cmake and feeding that in to other ports that want to use this deprecated findcuda.cmake script.

[simogasp]

Thanks for your replies.
Concerning the installation script, I don't know if the Cuda installation creates a /usr/local/cuda/ symlink, that's where FindCuda looks for the nvcc.
It seems to me that there is only a /usr/local/cuda10.2 which cannot be found by FindCuda as noted by @JackBoosY .
So adding a line like

sudo ln -s /usr/local/cuda-10.2 /usr/local/cuda

should make things better.
Also be careful as there is a 10.1 where I believe it should be a 10.2?

vcpkg/scripts/azure-pipelines/linux/provision-image.sh

Line 40 in 5a80a8e

sudo ln -s /usr/local/cuda-10.1/lib64/stubs/libcuda.so /usr/local/cuda-10.1/lib64/stubs/libcuda.so.1

But I agree that having a mechanism to export the cuda path would be great!

[simogasp]

@JackBoosY any idea why we are getting this error only for x86, the baseline for the version is correctly set
https://dev.azure.com/vcpkg/public/_build/results?buildId=49417&view=logs&j=0a61404f-5c45-5632-e83c-408b7fcca1d6&t=591b82ac-d7a4-522e-cf3c-66395e94b618&l=1619

[simogasp]

@JackBoosY sorry, scratch my previous question, I didn't see we need another json to be added. Now it seems to be working.
You can merge if you don't have any further comments (at long last!! :-)

[JackBoosY]

Wait for #16270 merge.

[JackBoosY]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JackBoosY]

LGTM in my side.

[simogasp]

Any news? Is the PR ok to be merged or are there any more comments? I know we kept it WIP for a very long time, it would be nice to merge it soon if no other changes are required. Thx!

[ras0219-msft]

Thanks for the ping to bring this back up to the top of the stack!