Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[colmap] Add port for COLMAP 3.6 #12410

Merged
merged 21 commits into from
Aug 7, 2020
Merged

[colmap] Add port for COLMAP 3.6 #12410

merged 21 commits into from
Aug 7, 2020

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 <[email protected]>
Co-authored-by: Robert Schumacher <[email protected]>
Co-authored-by: Robert Schumacher <[email protected]>
@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:reviewed Pull Request changes follow basic guidelines 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...

@JackBoosY JackBoosY removed requires:testing Needs tests added before merging requires:author-response labels Aug 4, 2020
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Aug 5, 2020
@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 6, 2020
@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:reviewed Pull Request changes follow basic guidelines 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:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants