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

Add option to install only BUILD_SHARED_LIBS-matching files #3081

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Oct 9, 2024

For Debian, I've been asked to get rid of the non-shared libraries that get installed by tdlib. This was previously mentioned in #1810, with the TL;DR there being "libtdjson_static must be static, the rest are internal and maybe shouldn't be installed".

Here is a shot at resolving this by:

  • adding an option() called TD_ENABLE_FILTERED_INSTALL (default OFF) to control if the installed libraries & library-supporting files should be limited to match BUILD_SHARED_LIBS

@levlam
Copy link
Contributor

levlam commented Oct 10, 2024

Adding the option BUILD_SHARED_LIBS explicitly is redundant and harmful and must never be done. This just breaks all projects that add the current project as subproject and doesn't add the option explicitly, which is described in the mentioned documentation.

tdmtproto, tdcore and other mentioned libraries are not suitable to be used as separate shared libraries and must not be used as such. libtdjson.so must be installed as a single shared library.

Also, see the last paragraph at #2696 (comment), which seems to be relevant.

If you prefer, another option can be added that installs only shared/static libraries if the option is enabled.

@OPNA2608
Copy link
Contributor Author

Adding the option BUILD_SHARED_LIBS explicitly is redundant and harmful and must never be done. This just breaks all projects that add the current project as subproject and doesn't add the option explicitly, which is described in the mentioned documentation.

The documentation explicitly mentions that projects often define this as an option(), so I'm not sure about calling this "must never be done" behaviour. If you wish to care about the use-cases you describe, then it should merely require some more careful handling no? I.e. only defining the option when the project is the top-level one would prolly work.

But sure, can not introduce this if absolutely undesired.

tdmtproto, tdcore and other mentioned libraries are not suitable to be used as separate shared libraries and must not be used as such.

Then they likely shouldn't be installed in the first place. If you're keeping them around to facilitate linking of the static tdjson, where the dependencies must be available & provided when linking against it, then maybe it would make sense to make them object libraries and only link them into one final user-available static library at the end?

libtdjson.so must be installed as a single shared library.

Even when BUILD_SHARED_LIBS:BOOL=OFF is set, which is the user explicitly telling you "I don't want shared libraries"?

If you prefer, another option can be added that installs only shared/static libraries if the option is enabled.

If always linking both variants is a requirement, then that would be a compromise I'm okay with.

@levlam
Copy link
Contributor

levlam commented Oct 10, 2024

The documentation explicitly mentions that projects often define this as an option(), so I'm not sure about calling this "must never be done" behaviour. If you wish to care about the use-cases you describe, then it should merely require some more careful handling no? I.e. only defining the option when the project is the top-level one would prolly work.

The documentation mentions this because other projects must be aware, that doing so can break building of the outer project, unless the outer project does the same. Hence, if you are forced to include such harmful subproject, then you must also become a harmful project. Otherwise, everything works by default correctly without any explicit options.

Then they likely shouldn't be installed in the first place.

They are needed for native C++ interface and libtdjson_static, which aren't usually used. That's why an option to install only the shared library without C++ headers can be added.

then maybe it would make sense to make them object libraries and only link them into one final user-available static library at the end

Object libraries can't be used in target_link_libraries() before CMake 3.12. Therefore, this can't be done without dropping support for older CMake versions and corresponding operating systems. Someday, this will be done, but not yet.

Even when BUILD_SHARED_LIBS:BOOL=OFF is set, which is the user explicitly telling you "I don't want shared libraries"?

No, it doesn't mean this. As any other options BUILD_SHARED_LIBS is false by default, hence, it means "build static libraries by default". It tells nothing about libraries like tdjson that must be shared, because otherwise they can't be used from most programming languages.

If always linking both variants is a requirement, then that would be a compromise I'm okay with.

The option can remove static/shared target libraries also, but this will take neglible effect on the build time.

@OPNA2608 OPNA2608 force-pushed the add/BUILD_SHARED_LIBS-compliance branch from 20eea4f to 502d330 Compare October 11, 2024 18:09
@OPNA2608 OPNA2608 changed the title Set & comply with BUILD_SHARED_LIBS Add option to install only BUILD_SHARED_LIBS-matching files Oct 11, 2024
TD_ENABLE_FILTERED_INSTALL allows limiting installed libraries & supporting files to only those matching
BUILD_SHARED_LIBS.
@OPNA2608 OPNA2608 force-pushed the add/BUILD_SHARED_LIBS-compliance branch from 502d330 to f14ab44 Compare October 11, 2024 21:06
@OPNA2608
Copy link
Contributor Author

I've introduced an option TD_ENABLE_FILTERED_INSTALL (feel free to suggest a better / change the name for this) to enable the discussed behaviour, defaulting to OFF to preserve current behaviour.

Listing of installed files for each relevant option combination
-DTD_ENABLE_FILTERED_INSTALL:BOOL=ON -DBUILD_SHARED_LIBS:BOOL=ON
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdjson.so.1.8.37
-- Set non-toolchain portion of runtime path of "/nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdjson.so.1.8.37" to ""
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdjson.so
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdclient.so.1.8.37
-- Set non-toolchain portion of runtime path of "/nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdclient.so.1.8.37" to ""
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdclient.so
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdapi.so.1.8.37
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/libtdapi.so
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/pkgconfig/tdclient.pc
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/pkgconfig/tdapi.pc
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/pkgconfig/tdjson.pc
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/cmake/Td/TdTargets.cmake
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/cmake/Td/TdTargets-release.cmake
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/td_json_client.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/td_log.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/tdjson_export.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/Client.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/Log.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/tl/TlObject.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/td_api.h
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/include/td/telegram/td_api.hpp
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/cmake/Td/TdConfig.cmake
-- Installing: /nix/store/s22jhr5ijix95nn9rf8ak4xxx643n0c7-tdlib-local/lib/cmake/Td/TdConfigVersion.cmake
-DTD_ENABLE_FILTERED_INSTALL:BOOL=ON -DBUILD_SHARED_LIBS:BOOL=OFF
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdjson_static.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdjson_private.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdcore.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdmtproto.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdclient.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdapi.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdutils.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdactor.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdnet.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdsqlite.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tddb.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdmtproto.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdcore.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdclient.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdapi.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdjson_private.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/pkgconfig/tdjson_static.pc
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/cmake/Td/TdStaticTargets.cmake
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/cmake/Td/TdStaticTargets-release.cmake
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/td_json_client.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/td_log.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/tdjson_export.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/Client.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/Log.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/tl/TlObject.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/td_api.h
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/include/td/telegram/td_api.hpp
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/cmake/Td/TdConfig.cmake
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/cmake/Td/TdConfigVersion.cmake
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdutils.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdactor.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdnet.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtdsqlite.a
-- Installing: /nix/store/6s62fqhgb93p1hbwvmjhmh6wm2l5314j-tdlib-local/lib/libtddb.a
-DTD_ENABLE_FILTERED_INSTALL:BOOL=OFF
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdjson.so.1.8.37
-- Set non-toolchain portion of runtime path of "/nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdjson.so.1.8.37" to ""
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdjson.so
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdclient.so.1.8.37
-- Set non-toolchain portion of runtime path of "/nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdclient.so.1.8.37" to ""
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdclient.so
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdapi.so.1.8.37
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdapi.so
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdjson_static.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdjson_private.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdcore.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdmtproto.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdutils.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdactor.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdnet.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdsqlite.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tddb.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdmtproto.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdcore.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdclient.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdapi.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdjson_private.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdjson.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/pkgconfig/tdjson_static.pc
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/cmake/Td/TdTargets.cmake
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/cmake/Td/TdTargets-release.cmake
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/cmake/Td/TdStaticTargets.cmake
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/cmake/Td/TdStaticTargets-release.cmake
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/td_json_client.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/td_log.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/tdjson_export.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/Client.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/Log.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/tl/TlObject.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/td_api.h
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/include/td/telegram/td_api.hpp
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/cmake/Td/TdConfig.cmake
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/cmake/Td/TdConfigVersion.cmake
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdutils.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdactor.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdnet.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtdsqlite.a
-- Installing: /nix/store/gjjvqv5vdkm2d8jkl3sx2173iknw8271-tdlib-local/lib/libtddb.a

@levlam
Copy link
Contributor

levlam commented Oct 21, 2024

Thank you, @OPNA2608!

I will merge the changes, despite I don't like that combination of options is required to achieve behavior changes and that the standard option BUILD_SHARED_LIBS is used for what it isn't supposed to be used.

Also, it is odd that behavior of generate_pkgconfig and libraries from subdirectories is affected by the top-level option TD_ENABLE_FILTERED_INSTALL, which makes hard to reuse them.

I will fix both issues myself after merging the commit.

@levlam levlam merged commit 1e380de into tdlib:master Oct 21, 2024
@tdlib tdlib deleted a comment from Scutua Oct 21, 2024
@levlam
Copy link
Contributor

levlam commented Oct 21, 2024

I replaced the option TD_ENABLE_FILTERED_INSTALL with separate options TD_INSTALL_STATIC_LIBRARIES and TD_INSTALL_SHARED_LIBRARIES, which can be used to control whether static/shared libraries are installed.

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