-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[alicevision] Add new port #38034
base: master
Are you sure you want to change the base?
[alicevision] Add new port #38034
Conversation
Waiting for liblzma issue to be fixed. |
@WangWeiLin-MV can you plz check whether |
Depends on #38071 |
@fabiencastan Can you please review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent needs to be unified for portfile.cmake
.
Waiting for the upstream review this PR. |
Is there a problem to enable the SFM? It's at the core of alicevision, so it will limit a lot the usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few remarks
SOURCE_PATH "${SOURCE_PATH}" | ||
OPTIONS | ||
-DALICEVISION_BUILD_SFM=OFF | ||
-DALICEVISION_BUILD_MVS=ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This on the other hand can be ON or OFF depending on the availability of CUDA (related to ALICEVISION_USE_CUDA
) and GPU on the machine. Maybe we can leave it as a documentation note, the user might be able to build but not to execute without an NVidia card
@JackBoosY have you seen the review from @simogasp ? |
@fabiencastan vacation now, will continue this PR several days later. |
@fabiencastan Alicevision does not provide Edit: I saw a copy of FindCoinUtils.cmake in https://github.com/open-anatomy/SfM_gui_for_openMVG/blob/master/src/cmakeFindModules/FindCoinUtils.cmake, no idea whether we can use this with license. |
Currently we are using a fork: https://github.com/alicevision/CoinUtils/commits/av_develop/ There is also another PR on the official coinutils that has not been merged to add cmake. The best would be to get something merged and maybe in the meantime add the cmage config in the vcpkg patch of coinuils. |
Since such ports now use makefile, we may need to wait for the upstream PRs or add vcpkg-wrapper & Find.cmake. |
That's unfortunate because we moved to that fork cmake-based system (we used the CMakeLists from vcpkg at that time) hoping that someday the official repos would also move to cmake. Since we use vcpkg for the Windows CI it also made sense. How do you consume those libs built with make in vcpkg now in a project with cmake? Do you have to provide your own FindX.cmake? |
Yeah we currently may pick one of two ways:
|
ok, but I meant more generally, if your project is cmake-based, how are you supposed to import a library built using |
|
|
I resolved the coinutils / clp /osi couldn't be found issue using pkgconfig, but got new issue about boost:
AFAIK, |
The error seems to come from Boost::regex which is missing here: |
No, boost-regex was already added into dependencies and found in |
|
In the find_package, we list all the modules that we will need globally in the project, but then we need to list the boost modules that we are using in each element, like here: |
Add
|
@fabiencastan @simogasp Any thought? |
Maybe we should replace |
I think |
This doesn't work since |
Could you please try using this PR : Briefly tested on msvc 2022. |
Will try later. |
Current: cannot find openmp on x64-windows, trying to fix this. |
@simogasp It's wired that openmp_cxx is not found in Windows but other ports which configured with openmp can find it, any idea on this? Edit: I tried to copy
But the same code doesn't work in
|
Tried with this PR changes but failed on link process:
|
That's very strange indeed. It's true that we have a useless project nesting and we should change that, but it should not be an issue when finding dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: I tried to copy
find_package(OpenMP)
to top ofroot/CMakeLists.txt
and openmp_cxx could be found:
To the top? Before project()
, languages and the corresponding cmake variables are not initialized. This doesn't deliver valid information.
CMakeFiles/CMakeConfigureLog.yaml
should be a good report of cmake configure checks. And there is --trace-expand
.
|
||
vcpkg_cmake_install() | ||
|
||
vcpkg_cmake_config_fixup(PACKAGE_NAME AliceVision CONFIG_PATH share/aliceVision/cmake) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vcpkg_cmake_config_fixup(PACKAGE_NAME AliceVision CONFIG_PATH share/aliceVision/cmake) | |
vcpkg_cmake_config_fixup(CONFIG_PATH share/aliceVision/cmake) |
Yeah, the thing is that we have 2 |
I don't complain about a second |
I mean add it after all |
I changed this to draft because the build is broken; please feel free to mark 'Ready for review' when it's working and you're happy with it |
@fabiencastan @simogasp Any help on this issue? |
Continue PR #8829
Fixes #8819.
Related: alicevision/AliceVision#767