Skip to content

[colmap] Add port for COLMAP 3.6#12410

Merged
strega-nil merged 21 commits intomicrosoft:masterfrom
pablospe:colmap
Aug 7, 2020
Merged

[colmap] Add port for COLMAP 3.6#12410
strega-nil merged 21 commits intomicrosoft:masterfrom
pablospe:colmap

Conversation

@pablospe
Copy link
Contributor

Add port for COLMAP 3.6-dev.3. Related to issue #8820.

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jul 14, 2020
…nd *.bat) to `tools/` otherwise I get `POST_BUILD_CHECKS_FAILED`. I followed this recommendation:

microsoft#834 (comment)

Now the *.bat files need to be fixed with the correct path to `tools/`
@pablospe
Copy link
Contributor Author

Last commit fixed some errors in portfile. These changes also move the binary (and *.bat) to tools/ folder, otherwise I get POST_BUILD_CHECKS_FAILED with these errors:

The following EXEs were found in /bin or /debug/bin. EXEs are not valid distribution targets.
    C:/build/vcpkg/packages/colmap_x64-windows/bin/colmap.exe
The following EXEs were found in /bin or /debug/bin. EXEs are not valid distribution targets.
    C:/build/vcpkg/packages/colmap_x64-windows/debug/bin/colmap.exe
Found 2 error(s). Please correct the portfile

I followed this recommendation and moved it to tools folder: #834 (comment)
But now the *.bat files need to be fixed with the correct path to tools/

A second problem is that when I run the binary I get: The code execution cannot proceed because boost_filesystem-vc142-mt-x64-1_73.dll was not found. This doesn't happen to me if I compile colmap with vcpkg following these instructions, perhaps the install target is the difference. I was playing with the RPATH (see this link) to see if I could fixed but it didn't make any difference.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

pablospe and others added 3 commits July 17, 2020 09:40
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@pablospe
Copy link
Contributor Author

@ras0219-msft, here you were suggesting to use vcpkg_copy_tools():
#12410 (comment)

but the sqlite example doesn't use vcpkg_copy_tools(). Are these the lines you refer to?

vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    INVERTED_FEATURES
    tool SQLITE3_SKIP_TOOLS
)

I think this will require some changes in the COLMAP code, to avoid compiling the binaries tools using a variable that can be disable here. Anyways I would assume that the regular user of COLMAP would expect the binaries to be compiled by default.

pablospe added 2 commits July 17, 2020 13:12
…UTO_CLEAN)` helped.

Now it does work running:

    > <vcpkg-root>\vcpkg\packages\colmap_x64-windows\tools\colmap\colmap.exe
    > <vcpkg-root>\vcpkg\packages\colmap_x64-windows\tools\colmap\colmap.exe gui

ToDo: use `vcpkg.json`.
@pablospe
Copy link
Contributor Author

Please try:

<vcpkg-root>\vcpkg.exe install colmap[cuda] --triplet x64-windows

Now it should work:

<vcpkg-root>\vcpkg\packages\colmap_x64-windows\tools\colmap\colmap.exe
<vcpkg-root>\vcpkg\packages\colmap_x64-windows\tools\colmap\colmap.exe gui

@pablospe pablospe requested a review from ras0219-msft July 18, 2020 22:49
./vcpkg.exe x-format-manifest --all
Copy link
Contributor Author

@pablospe pablospe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the changes were incorporated, except that I still need to remove some empty folders

@pablospe
Copy link
Contributor Author

qt5-imageformats:x86-windows fails to build. Any idea what to do in these cases? For the rest architectures seems to work (x64_windows, x64_linux, etc.).

qt5-imageformats:x86-windows: BUILD_FAILED

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jul 20, 2020
@ahojnnes
Copy link
Contributor

ahojnnes commented Jul 20, 2020

@pablospe I ran into a few issues compiling this on my machine.

I made a few commits yesterday on the COLMAP side to fix them.

In addition, I suggest to incorporate the following improvements:

portfile:

if("cuda-redist" IN_LIST FEATURES)
    set(CUDA_ENABLED ON)
    set(CUDA_ARCHS "Common")
endif()

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS
        -DCUDA_ENABLED=${CUDA_ENABLED}
        -DCUDA_ARCHS=${CUDA_ARCHS}
)

vcpkg.json

  "features": [
    {
      "name": "cuda",
      "dependencies": [
        "cuda"
      ],
      "description": "CUDA support for current compute architecture of this machine"
    },
    {
      "name": "cuda-redist",
      "dependencies": [
        "cuda"
      ],
      "description": "Redistributable CUDA support for common supported compute architectures"
    }
  ]

usage:

For example, under Windows, execute COLMAP as:

    .\packages\colmap_x64-windows\tools\colmap\colmap.exe gui
    .\packages\colmap_x64-windows\tools\colmap\colmap.exe mapper
    .\packages\colmap_x64-windows\tools\colmap\colmap.exe ...

The package colmap provides CMake integration:

    find_package(COLMAP REQUIRED)
    target_link_libraries(main ${COLMAP_LIBRARIES})

@ahojnnes
Copy link
Contributor

The cuda-redist feature allows one to compile a redistributable COLMAP version with CUDA compiled to different GPU architectures.

@ahojnnes
Copy link
Contributor

In addition, do you know, if there is a way to tell vcpkg to compile the latest commit from a specific branch from Github instead of checking out the predefined commit here?

@ahojnnes
Copy link
Contributor

Another comment: COLMAP automatically extracts version information from the git log at compile time using cmake/GenerateVersionDefinitions.cmake. It's not super important, but at the moment, this will produce a "Unknown" version number.

@ahojnnes
Copy link
Contributor

In addition, do you know, if there is a way to tell vcpkg to compile the latest commit from a specific branch from Github instead of checking out the predefined commit here?

Okay, answered this myself. Can be done with install --head.

…elease (probably today) to update the `REF` and `SHA512`
@pablospe
Copy link
Contributor Author

Another comment: COLMAP automatically extracts version information from the git log at compile time using cmake/GenerateVersionDefinitions.cmake. It's not super important, but at the moment, this will produce a "Unknown" version number.

To know the GIT_COMMIT_ID in colmap/cmake/GenerateVersionDefinitions.cmake it is possible to use "-D..." option with $VCPKG_HEAD_VERSION variable. But I don't know if there is a variable for the GIT_COMMIT_DATE. Any idea? @ras0219-msft, @JackBoosY.

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ahojnnes
Copy link
Contributor

@JackBoosY Seems like the CI build failures are not caused by this PR but by some unrelated Azure issues?

@JackBoosY
Copy link
Contributor

Testing...

pablospe and others added 2 commits August 4, 2020 09:56
@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Aug 5, 2020
@JackBoosY JackBoosY added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Aug 6, 2020
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
@JackBoosY
Copy link
Contributor

Waiting for merge #12766.

@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Aug 7, 2020
@JackBoosY JackBoosY requested a review from strega-nil August 7, 2020 02:20
@strega-nil strega-nil merged commit 6289ef0 into microsoft:master Aug 7, 2020
@pablospe pablospe deleted the colmap branch August 7, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:new-port The issue is requesting a new library to be added; consider making a PR! info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants