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

Auto generate tweakme.h #3077

Open
xvitaly opened this issue Apr 26, 2024 · 17 comments
Open

Auto generate tweakme.h #3077

xvitaly opened this issue Apr 26, 2024 · 17 comments

Comments

@xvitaly
Copy link
Contributor

xvitaly commented Apr 26, 2024

It looks like the include/spdlog/tweakme.h file should be automatically generated by CMake depending on the build configuration.

Various Linux distributions need to manually patch it in downstream to resolve issues with packages not using pkg-config or CMake.

@tt4g
Copy link
Contributor

tt4g commented Apr 26, 2024

It looks like the include/spdlog/tweakme.h file should be automatically generated by CMake depending on the build configuration.

CMake shouldn't generate tweakme.h, what makes you think it does?

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 26, 2024

CMake shouldn't generate tweakme.h, what makes you think it does?

I think it should be auto-generated for feature parity with pkg-config and CMake configurations.

@tt4g
Copy link
Contributor

tt4g commented Apr 26, 2024

Sorry. I am not a native speaker so I misunderstood the meaning.

spdlog was initially a header-only library. The option to build as a shared or static library was added later.
I believe that when it was header-only, there was no automatic tweakme.h generation because there was no significant difference between downloading and installing.
It is technically possible to generate tweakme.h at build time.
However, this may cause inconvenience for some users who still use spdlog as a header-only library and they have multiple projects using spdlog (even though spdlog is installed as a library, users can use it as a header-only library with a CMake target named spdlog::spdlog_header_only).

The final decision must be made by the maintainer @gabime.

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 26, 2024

However, this may cause inconvenience for some users who still use spdlog as a header-only library and they have multiple projects using spdlog (even though spdlog is installed as a library, users can use it as a header-only library with a CMake target named spdlog::spdlog_header_only).

Auto-generation can be enabled only if the SPDLOG_BUILD_SHARED flag is set.

@tt4g
Copy link
Contributor

tt4g commented Apr 26, 2024

spdlog::spdlog_header_only target is added with or without the SPDLOG_BUILD_SHARED flag.
When installed by CMake, spdlog can be either header-only and static/shared library.

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 26, 2024

When installed by CMake, spdlog can be either header-only and static/shared library.

Yes, but it has major issues with packages not using pkg-config or CMake. For more information please check this ticket.

@gabime
Copy link
Owner

gabime commented Apr 26, 2024

I’ve been working on something like this in the v2.x branch. cmake would generate a config.h file. Not sure yet if this the right approach though..

@tt4g
Copy link
Contributor

tt4g commented Apr 26, 2024

If this is considered a major change, it would make sense to automatically generate a configuration file from v2.
I personally think it is better to keep the style of macro definition by the developer as before, as it may confuse some users to automatically generate tweakme.h in v1.

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 26, 2024

cmake would generate a config.h file. Not sure yet if this the right approach though..

Looks good. Is it possible to backport this to 1.x?

@gabime
Copy link
Owner

gabime commented Apr 26, 2024

Porting it to v1.x would break current usage of tweakme.h or add confusion if generate a new file in addition to tweakme.h.

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 26, 2024

Porting it to v1.x would break current usage of tweakme.h or add confusion if generate a new file in addition to tweakme.h.

Even if it will be optional and disabled by default? SPDLOG_GENERATE_CONFIG for example. This feature can be very useful for Linux package maintainers.

If enabled, the config.h file will be generated and installed instead of tweakme.h.

@gabime
Copy link
Owner

gabime commented Apr 26, 2024

How would the new config.h be included by the code? Also note that when consuming spdlog
using cmake find_ pacakge, cmake already adds the right defines(e.g. target_compile_definitions(spdlog PUBLIC SPDLOG_SHARED_LIB) ,so what would happen if both methods used. Still seems confusing.

@tt4g
Copy link
Contributor

tt4g commented Apr 27, 2024

If a macro was defined when the library was built, CMake will define the same macro for the application to be linked.
If the same macro is also defined in config.h, the macro definitions will be duplicated, but as long as the definitions are the same, this should not be a problem.

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 27, 2024

How would the new config.h be included by the code?

Something like that:

#if __has_include(<spdlog/config.h>)
#include <spdlog/config.h>
#else
#include <spdlog/tweakme.h>
#endif

Also note that when consuming spdlog using cmake find_ pacakge, cmake already adds the right defines(e.g. target_compile_definitions(spdlog PUBLIC SPDLOG_SHARED_LIB) ,so what would happen if both methods used. Still seems confusing.

Yes, but we're talking about a situation where the packages don't use pkg-config or CMake.

@gabime
Copy link
Owner

gabime commented Apr 27, 2024

__has_include is c++17 while spdlog v1.x is c++11

@xvitaly
Copy link
Contributor Author

xvitaly commented Apr 27, 2024

__has_include is c++17 while spdlog v1.x is c++11

Another option is to install the generated config.h file under the name tweakme.h if the SPDLOG_GENERATE_CONFIG flag is enabled.

@gabime
Copy link
Owner

gabime commented Apr 27, 2024

Perhaps a SPDLOG_GENERATE_TWEAKME_Hcmake option (default=OFF) is indeed good way to solve this for 1.x.

PR is welcme.

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