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

allow devendoring of spdlog #60

Open
dvzrv opened this issue Jun 4, 2020 · 5 comments
Open

allow devendoring of spdlog #60

dvzrv opened this issue Jun 4, 2020 · 5 comments

Comments

@dvzrv
Copy link

dvzrv commented Jun 4, 2020

Hi! I'm maintaining osmid for Arch Linux.

As on most Linux distributions, we already have spdlog (and in a much newer version) it would be great to allow proper devendoring of the library (e.g. via CMake option).

Vendoring (aka bundling) of libraries (especially very old ones) is never a good idea. Fedora has a good writeup as to why that is.

The spdlog library offers both pkgconfig and cmake integration out-of-the-box, so discovery and integration of a system version should be trivial.

@llloret
Copy link
Owner

llloret commented Jun 4, 2020

Ok, makes sense, but do you have any advice about what to do for Windows, then?

@dvzrv
Copy link
Author

dvzrv commented Jun 4, 2020

Yeah, Windows is always a bit of a special case, I guess. ;-)

My proposal is to allow devendoring, not to enforce it. For environments such as Windows it could still remain vendored. However, other operating systems such as Linux have different requirements.

If allowing to use a system version of spdlog is done via a CMake option (e.g. USE_SYSTEM_SPDLOG), it should be fairly trivial: just default to OFF).
If it is ON, cmake discovers and uses the system version, if it is OFF the vendored version is used.
If the target include paths are set correctly there should not even be any difference for the code (and all can be set to global includes).

Closing, I guess it would also be great to upgrade the vendored version (to remain in sync with its upstream).

@llloret
Copy link
Owner

llloret commented Jun 4, 2020

@dvzrv , ok, I have put the change in the branch spdlog_from_distro. Can you please have a look and let me know if that is what you were expecting?

I have also updated the bundled spdlog in that branch to make it compatible with the one I see packaged for my Linux distribution, so that I can use the same source code in the application.

If this is what you were looking for, I'll merge into master and tag a new release.

@dvzrv
Copy link
Author

dvzrv commented Aug 10, 2020

Argh, sorry... I forgot about this ticket!
I'll look into this tomorrow for sure as I have to update the package... :)

@dvzrv
Copy link
Author

dvzrv commented Aug 11, 2020

Okay, I have commented on a small issue with the fix.

Unfortunately we are already at spdlog 1.7.0 and building against it does not work:
osmid-0.8.0-devendor_spdlog.txt

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

2 participants