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

System libs for glog and Ceres in CMake #829

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Conversation

YakoYakoYokuYoku
Copy link
Member

PR Description

What type of PR is this? (Check one of the boxes below)

  • New feature (non-breaking change which adds functionality)

What does this pull request do?

Because a system installation of glog may conflict with the bundled version (see #807) and Ceres can take quite the time to build, I've decided to provide the option to use the system version for the CMake build. openMVG was ignored due to not many distributions provide a package for it and its lack of CMake config files for the build to discover its install, though this can be revisited in the future.

Show a few screenshots (if this is a visual change)

N/A.

Have you tested your changes (if applicable)? If so, how?

By building with those libraries and running Natron.

Futher details of this pull request

In the future, when Qt6 support arrives alongside the official HTTP server module, qhttpserver will receive the same treatment.

@devernay
Copy link
Member

hum I don't think this is a good idea.
We're using an older version of Ceres, and we may be lagging on glog too. Current versions are untested. It's better to stick with what we provide until the current versions are properly tested.
The problem with a system installation of glog or ceres can be fixed by making sure the include path and the library path are correctly set. I would prefer that.

@YakoYakoYokuYoku
Copy link
Member Author

We're using an older version of Ceres, and we may be lagging on glog too. Current versions are untested. It's better to stick with what we provide until the current versions are properly tested.
The problem with a system installation of glog or ceres can be fixed by making sure the include path and the library path are correctly set. I would prefer that.

In my case I can say that current versions of Ceres and glog work with Natron in Arch Linux. But if issues raise from using the system installs then the bundled versions can comfortably be used instead, and due to the lack of testing in platforms different than Linux then it makes sense that the option is OFF by default as this PR does.

@devernay
Copy link
Member

Ok to keep it OFF, but does this also fix #807 if there are system versions installed?

@YakoYakoYokuYoku
Copy link
Member Author

YakoYakoYokuYoku commented Jul 23, 2022

I speculate that given the option to not use the bundled version of glog it would not pass any include or link flags that collide with the ones from a system install, but we have to take in consideration that QMake was being in use in that issue, which links glog as -lglog (as of time of writing of said issue CMake wasn't supported yet) and that CMake is a fix too, due to its nature that project libraries are linked using their path directly, i.e. glog would be linked with build/glog/libglog.a instead of -lglog, thus disambiguating between a bundled version and a system version of glog, same as openMVG or libmv.

@YakoYakoYokuYoku
Copy link
Member Author

Merging this PR in the evening if there are no further objections.

@YakoYakoYokuYoku YakoYakoYokuYoku merged commit 6588ac0 into RB-2.5 Jul 25, 2022
@YakoYakoYokuYoku YakoYakoYokuYoku deleted the system-glog-ceres branch July 25, 2022 16:44
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