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

please add a use_sys_sqlite3 config option #10263

Closed
simevo opened this issue Aug 27, 2023 · 5 comments
Closed

please add a use_sys_sqlite3 config option #10263

simevo opened this issue Aug 27, 2023 · 5 comments
Labels
enhancement a request to enhance doxygen, not a bug install/build bug in the installation or build scripts

Comments

@simevo
Copy link

simevo commented Aug 27, 2023

While working at packaging 1.9.8 for Debian, I noticed that you have vendored (bundled) spdlog and sqlite3 in deps/ dir.

There dependencies are available on all platforms supported by Debian, so this is not required for us (but I understand your point of course).

Debian is against vendoring so we'll repackage doxygen stripping these source files (that we already have in the corresponding packages), so for us the source tree has the dirs deps/spdlog and deps/sqlite3 missing.

With use_sys_spdlog config option I can tell CMake to skip the former, but it errors out because it still lacks the latter.

So for us it would be really helpful if you could add a use_sys_sqlite3 option and patch deps/CMakeLists.txt so:

 if (NOT use_sys_spdlog)
 	add_subdirectory(spdlog)
 endif()
+if (NOT use_sys_sqlite3)
+	add_subdirectory(sqlite3)
+endif()
-add_subdirectory(sqlite3)
simevo pushed a commit to simevo/doxygen that referenced this issue Aug 27, 2023
@albert-github albert-github added enhancement a request to enhance doxygen, not a bug install/build bug in the installation or build scripts labels Aug 27, 2023
@albert-github
Copy link
Collaborator

Having the possibility to used the OS distributed packages or the doxygen bundled has an advantage for a single user as he has the freedom to choose the version of a package he wants.
Removing a part from the distribution is not a good idea as this removes the, easy, possibility to reproduce the results from doxygen as they were at the moment of of the release.
Having the packages bundled has the advantage that the results are reproducible even when rebuilding by the user and also that it is always known which version of a bundled package is used.

When using the OS version of a package, probably some compilation link flags have to be set as well, wouldn't it be good to use the find_package / FindSQLite3.cmake (probably this would be good for spdlog as well although here it is an include file only that might be found in the system path when not coming from doxygen).

I don't think it is good that an OS provides an executable that is not build with the bundled parts.

Do not link proposed pull requests to the issue
image

as this is not the way doxygen handles issues / PRs, instead use a text e.g. like:
I've just pushed a proposed patch, pull request #10264

(in this case the I is not correct as @simevo created the PR).

@simevo
Copy link
Author

simevo commented Aug 28, 2023

Let me quote myself here:

While packaging for Debian and other distros, bundling should be avoided (if possible), a good synthesis of the rationale is here: https://wiki.gentoo.org/wiki/Why_not_bundle_dependencies.

But I understand that your priorities are different.

Back to this specific issue, I just provisionally patched like this:
https://salsa.debian.org/debian/doxygen/-/blob/master/debian/patches/exclude_sqlite3.diff
and built with the OS distributed version of sqlite3 library, which happens to be the same version that your currently bundle: 3.42.0, although this may change in the future.

Building works fine without setting any additional compilation link flags (CMake magick!).

I used the branch sqlite3 in this test repo here and the package built in the Debian salsa CI:
https://salsa.debian.org/debian/doxygen/-/jobs/4614916/artifacts/browse/debian/output/
like this to test:

git clone https://github.com/simevo/hello_doxygen.git
cd hello_doxygen
git checkout sqlite3
docker run --rm -it -v "$PWD:/hello_doxygen" -w /hello_doxygen debian:unstable
apt update && apt install -y wget
wget https://salsa.debian.org/debian/doxygen/-/jobs/4614916/artifacts/raw/debian/output/doxygen_1.9.8+ds-1+salsaci+20230827+81_amd64.deb
apt install ./doxygen_1.9.8+ds-1+salsaci+20230827+81_amd64.deb graphviz sqlite3
doxygen

The resulting sqlite file seems OK as this indicates:

ls -l sqlite3/
total 108
-rw-r--r-- 1 root root 110592 Aug 28 06:32 doxygen_sqlite3.db
sqlite3 sqlite3/doxygen_sqlite3.db 
select * from memberdef;
2|main|int main|int|()||||||0|0|0|0|0|0|0|0|0|0|0|0|0||||||||||||||||||||||||function|6|8|1|1|6|5|<para>... text ... </para>
||

Finally, regarding the automatic link to the PR to the issue, this happened because I tagged the issue with one of the supported keywords (fixes #...) in the commit message as documented here: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue. Sorry, it's an habit I have from working with other projects!

@albert-github
Copy link
Collaborator

Finally, regarding the automatic link to the PR to the issue, this happened because I tagged the issue with one of the supported keywords (fixes #...)

Auch those automatic things...
I normally also put a small description with the PR as this is what goes into git and when not having GitHub available but just the repository one still has a small indication of the problem.

@doxygen
Copy link
Owner

doxygen commented Aug 28, 2023

@simevo Please check if the referenced commit works for you. Do not close the issue, this will be done automatically when the next official release is out.

@albert-github albert-github added the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label Aug 29, 2023
@doxygen
Copy link
Owner

doxygen commented Dec 25, 2023

This issue was previously marked 'fixed but not released',
which means it should be fixed in doxygen version 1.10.0.
Please verify if this is indeed the case. Reopen the
issue if you think it is not fixed and please include any additional information
that you think can be relevant (preferably in the form of a self-contained example).

@doxygen doxygen removed the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label Dec 25, 2023
@doxygen doxygen closed this as completed Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a request to enhance doxygen, not a bug install/build bug in the installation or build scripts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants