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

Always use external CoinUtils, Osi and Clp libraries #1237

Merged
merged 4 commits into from
Sep 28, 2022

Conversation

p12tic
Copy link
Contributor

@p12tic p12tic commented Sep 23, 2022

Previously we used an internal fork of CoinUtils, Osi and Clp libraries that was included as a submodule. The repository structure was significantly different than what upstream has (3 repositories vs. 1 on our side) and thus tracking upstream was harder than necessary.

This PR converts the project to use new forks of upstream CoinUtils, Osi and Clp at https://github.com/alicevision/CoinUtils, https://github.com/alicevision/Osi and https://github.com/alicevision/Clp. These repositories currently have pending PRs with cmake support.

@simogasp
Copy link
Member

simogasp commented Sep 23, 2022

[note to self] we will also need to update and upload the docker images as they are used for the ci

@servantftechnicolor
Copy link
Contributor

Very cool, this task was in my backlog. Will reduce the rebuild time of alicevision.

@fabiencastan
Copy link
Member

I'm rebuilding the docker image for the dependencies to be able to use it in the linux CI.

@p12tic
Copy link
Contributor Author

p12tic commented Sep 26, 2022

I've rebased on top of latest develop branch and pinned the dependencies to the exact commits like is done for the rest of dependencies.

@p12tic p12tic marked this pull request as ready for review September 26, 2022 17:17
@p12tic p12tic changed the title Draft: Always use external CoinUtils, Osi and Clp libraries Always use external CoinUtils, Osi and Clp libraries Sep 26, 2022
@fabiencastan
Copy link
Member

I get this error when building it with ./docker/build-centos.sh:

CMake Error at /usr/local/share/cmake-3.21/Modules/BundleUtilities.cmake:471 (file):
  file READ_ELF given FILE "libClp.so" that does not exist.
Call Stack (most recent call first):
  /usr/local/share/cmake-3.21/Modules/BundleUtilities.cmake:527 (get_item_rpaths)
  /usr/local/share/cmake-3.21/Modules/BundleUtilities.cmake:649 (set_bundle_key_values)
  /usr/local/share/cmake-3.21/Modules/BundleUtilities.cmake:934 (get_bundle_keys)
  /opt/AliceVision_git/src/cmake/MakeBundle.cmake:138 (fixup_bundle)

@p12tic
Copy link
Contributor Author

p12tic commented Sep 27, 2022

@fabiencastan I've fixed the fixup_bundle error. The install prefix was not specified.

@fabiencastan
Copy link
Member

We will need to add osi to vcpkg.

@simogasp
Copy link
Member

We will need to add osi to vcpkg.

https://github.com/microsoft/vcpkg/tree/master/ports/osi

@@ -174,6 +174,8 @@ jobs:
ceres[suitesparse,cxsparse]
tbb
assimp
clp
Copy link
Member

Choose a reason for hiding this comment

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

clp will automatically import coinutils and osi

@simogasp simogasp added this to the 2.5.0 milestone Sep 27, 2022
@fabiencastan
Copy link
Member

-- FLANN: 1.8.4 (internal)
-- CLP: 
-- COINUTILS: 
-- OSI: 
-- LEMON:  (internal)

Missing XX_VERSION variables.

src/CMakeLists.txt Outdated Show resolved Hide resolved
@fabiencastan fabiencastan merged commit f987354 into alicevision:develop Sep 28, 2022
@p12tic p12tic deleted the remove-coin-submodule branch September 29, 2022 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants