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

Installing QuickQanava through CMake seems to be broken #165

Closed
sheepy9 opened this issue Sep 19, 2022 · 5 comments
Closed

Installing QuickQanava through CMake seems to be broken #165

sheepy9 opened this issue Sep 19, 2022 · 5 comments

Comments

@sheepy9
Copy link

sheepy9 commented Sep 19, 2022

Installing QuickQanava using cmake seems to be broken. After running make install, the qml files and binaries seem to be installed in the correct place, but the header files are unusable in the current configuration.
Some headers like for example container_adapter.h (amoung many) use #include with relative paths to other headers like QuickContainers.h. This is a problem since the directory structure of the installed headers differs from the directory structure inside the QuickQanava project. And so when building QuickQanava, no compiler errors are reported, while it's impossible to include QuickQanava.h in another project after installation.

I managed to fix this while integrating QuickQanava into my project, but along the way I also made some other (exclusively build system) changes.

Generally it's nice to have the ability to build and install headers, binaries, cmake package files and qml files of dependencies locally inside a projects directory tree (like KDE modules which have this good practice).
What I mean by this is that it would be good to be able to change CMAKE_INSTALL_PREFIX from the default /usr/local to some subdirectory in your project and have everything loaded from there. This would make it very straightforward to use QuickQanava by building it directly from github using ExternalProject_Add. And since it would be installed locally, no admin privileges would be required in the install step. Then it can easily be included using find_package etc. This also makes it easier to later package the project in question.

I managed to enable this and get QuickQanava working in my project so I was thinking that something like this would also be generally desirable. If you want I could contribute these modifications to QuickQanava.
All changes are made exclusively to CMake files, no source or header files were changed.

@cneben
Copy link
Owner

cneben commented Nov 28, 2022

Hi @sheepy9 thanks for your report, feel free to PR your changes, but integration strategy really focus on git submodules and static linking; old style "packaging" using "make install" is really not recommended and has actually never been configured.

@emmenlau
Copy link
Contributor

emmenlau commented Jan 27, 2023

I've just been hit by this problem, that QuickQanava seems not to support a normal install. At least it seems it can not be used as a C++ library after install. After install, even the most basic header can not be included, because it references other headers in paths that can not be resolved.

I'm under the impression that this could be quite easily fixed in QuickQanava. I think I could propose CMakeLists and changes in includes that make QuickQanava work nicely both when installed but also when used as a submodule. Would that be something you could consider for integration?

Can I ask why QuickQanava uses include paths that are actual paths, starting with ./? That is rather uncommon, and seems not required. Usually cmake should set the paths correctly for the compiler? Would it be acceptable to change this?

@cneben
Copy link
Owner

cneben commented Jan 29, 2023

Thanks for your contribution @emmenlau ! I still do not recommand to make install versus a direct submodule inclusion, but I will merge ASAP !

@emmenlau
Copy link
Contributor

Awesome @cneben, I'm more than happy if the changes work for you and can be accepted! Also, I would not mind keeping this up-to-date every once in a while. And the changes are anyways pretty small...

@emmenlau
Copy link
Contributor

Kindly pinging this issue again...

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

No branches or pull requests

3 participants