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

Remove deprecated register modifier #3

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

p12tic
Copy link
Contributor

@p12tic p12tic commented Sep 20, 2022

register modifier has been removed in C++17.

This has already been addressed upstream:

coin-or/CoinUtils@1700ed9

cc @fabiencastan

register modifier has been removed in C++17.

This has already been addressed upstream:

coin-or/CoinUtils@1700ed9
@p12tic
Copy link
Contributor Author

p12tic commented Sep 20, 2022

@fabiencastan I think it would make sense to properly fork upstream repositories and add these as dependencies via git submodules. The current approach of having a repository that does not correspond to any of the upstream repositories is not optimal.

Basically I propose that we have the following 3 submodules in AliceVision:

@simogasp
Copy link
Member

The problem is that the original repos are not based on cmake, so that makes it difficult to build on windoze. That's why this repo was created to combine the 3 in a single one and a basic cmake support. We can look at vcpkg, how they created the cmake support there and maintain a version of the 3 with the patches provided by vcpkg. Just to avoid to redo and maintain yet another version for the 3

@p12tic
Copy link
Contributor Author

p12tic commented Sep 20, 2022

@simogasp I that maintaining effort is the same regardless of whether we have 3 or 1 repository. Having said that I agree that there will be significant cost during switchover. I volunteer to implement the switch though, so this cost shouldn't matter too much to you.

Would you fork these 3 repositories to AliceVision organization and accept my PRs if I get everything to work?

@p12tic
Copy link
Contributor Author

p12tic commented Sep 20, 2022

@simogasp I decided not to wait for your answer and see how far I can get if I just went to implement the conversion. It turns out it was much easier than I thought thanks to your suggestion to use vcpkg as a source. The CMake files needed some non-obvious changes to be able to fit into AliceVision as subprojects, but other than that everything was straightforward. AliceVision unit tests pass.

If you forked the repositories I listed to alicevision organization I would submit PRs implementing CMake functionality and then switch main AliceVision project to the new dependencies.

@simogasp
Copy link
Member

great! I think if we can successfully build the 3 on Linux and windows it will be even better to remove the subproject altogether and use them only as external libs like any other lib. Nothing good comes in including another CMake project in my experience. And it will simplify the management of the lib (internal/external) and hopefully, one day, we will be able to add the vcpkg port of AV more smoothly

@simogasp
Copy link
Member

simogasp commented Sep 21, 2022

They are forked
https://github.com/AliceVision/CoinUtils etc
Each one has a av_develop branch set as the default one on which we can make our version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants