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

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

Closed
wants to merge 2 commits into from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jun 22, 2024

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:

  • 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).

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.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@valgur valgur force-pushed the feature/tweak_deployers branch from b292854 to 8b392a9 Compare June 22, 2024 19:34
@memsharded
Copy link
Member

Thanks for your contribution @valgur

The runtime_deployer from #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:

  • Focus on fixing things first, if only *.so are being copied and missing .so.1.3.1, then lets write a short unittest that reproduces it, and then fix that. This can go in its own PR
  • If the symlinks is totally related to the first point, it might go into its own PR. Otherwise it can go in the same PR. As above, the best way would be to provide a simple unittest that fires and triggers the reported bug. Otherwise changes are not guaranteed to work and keep working in the future.

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.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the notes in #16527 (comment), this PR is doing many different things, so better:

  • Use different PR for bug fixes
  • Include tests first to show those bugs and guarantee future correctness and robustness

@valgur
Copy link
Contributor Author

valgur commented Jun 27, 2024

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

@valgur valgur mentioned this pull request Jul 3, 2024
9 tasks
@memsharded
Copy link
Member

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.

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