conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.25k stars 980 forks source link

Replace `runtime_deploy` with `shared_deploy` and `merged_deploy`, refactor a bit #16527

Closed valgur closed 2 months ago

valgur commented 4 months ago

Changelog: Feature: Added new merged_deploy and shared_deploy deployers Docs: https://github.com/conan-io/docs/pull/XXXX

The runtime_deployer from #15382 is a very good addition to Conan, but the implementation currently has some defects:

Also, it's debatable, but I would say copying all runtime executables and possibly plugin dynamic libs into a single flat directory in addition to just shared libs is of questionable value. In a typical setup this will include a lot of irrelevant binaries like lzcat, lzcmp, lzdiff, lzegrep, lzfgrep, lzgrep, lzless, lzma, lzmadec, lzmainfo, lzmore, pbmtojbg, unlzma, unxz, unzstd, xz, xzcat, xzcmp, xzdec, xzdiff, xzegrep, xzfgrep, xzgrep, xzless, xzmore, zstd, zstdcat, zstdgrep, zstdless, zstdmt from just the compression libs alone. For the most common use cases just the shared libraries from the dependencies should be sufficient. Once you start requiring executables or plugins, you quite possibly also need data from res as well and what not. In that case copying exactly what you need would be a better idea, IMO.

With that in mind I replaced the runtime_deployer with shared_deployer and added merged_deployer. The former copies just the shared host dependency libraries to <deployer-folder>. The latter is similar to full_deployer but merges all package folders into a single one under <deployer-folder>/merged_deployer. This makes re-packaging of Conan dependencies for distribution much more convenient.

If possible, I would like the shared_deployer to be even more specific, copying only libraries from self.cpp_info.requires, but I could not figure out how this could be implemented with a reasonable amount of complexity.

I have not updated the unit tests or docs. Consider this more of a proposal for now. Let me know what you think.

memsharded commented 4 months ago

Thanks for your contribution @valgur

The runtime_deployer from https://github.com/conan-io/conan/pull/15382 is a very good addition to Conan, but the implementation currently has some defects:

  • Only .so files (e.g. libz.so) are copied, but not .so.* (e.g. libz.so.1 and libz.so.1.3.1).
  • shutil.copy2(..., follow_symlinks=symlinks) should be shutil.copy2(..., follow_symlinks=not symlinks).

I think that we should focus in these bugs first, and try to keep changes to a very minimum. Some tests are being broken:

FAILED test/functional/command/test_install_deploy.py::test_builtin_full_deploy FAILED test/functional/command/test_install_deploy.py::TestRuntimeDeployer::test_runtime_deploy FAILED test/functional/command/test_install_deploy.py::TestRuntimeDeployer::test_runtime_deploy_components

I'd suggest:

Then, I agree that other generators that do something different might be relevant, and we can discuss its value. Note that it is not critical that all possible variants need to be built-in. Deployers are also user-customizable, if they don't want the executables, they can easily create their own deployer from the built-in one and remove those executables. But this should be discussed in its own ticket, I'd prioritize this one to focus on the bugs.

valgur commented 4 months ago

Not that it's a blocker, but regarding writing unit tests first, please consider sharing your thoughts on #16550.

memsharded commented 2 months ago

Closing this as not planned, as suggested above the way to move forward is new focused different PRs for bugs and for new features. Also not breaking existing users if possible, so runtime_deploy better to stay with that name (it can have bug fixes if those are real bugs), and new deployers might be better incubated in conan-extensions first. Thanks for your contribution.