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 built in runtime deployer #15382

Merged
merged 9 commits into from
Jun 12, 2024

Conversation

fdgStilla
Copy link
Contributor

@fdgStilla fdgStilla commented Jan 3, 2024

Changelog: Feature: Add built in runtime_deploy deployer.
Docs: conan-io/docs#3789

Fix #15381

This PR is currently a draft, only tested manually on windows with:

conan install --requires=zlib/1.3 --requires=cmake/3.28.1 -of C:\Users\florian.degaulejac\tmp\zlib-cmake --deployer runtime_deploy.py -vtrace --deployer-folder C:\Users\florian.degaulejac\tmp\zlib-cmake\deploy -o "zlib/*:shared=True"

Remaining tasks that I won't have the time to complete:

  • write autotest instead of manual test
  • test other env (linux, apple...)
  • symbolic links
  • test with deep hierarchy and check it is flatten

And a weird behavior I observed: dep.cpp_info.bindirs and dep.cpp_info.libdirs are populated with absolute paths instead of relative path like described in the documentation.

Docs: https://github.com/conan-io/docs/pull/XXXX

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

@CLAassistant
Copy link

CLAassistant commented Jan 3, 2024

CLA assistant check
All committers have signed the CLA.

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.

Thanks for contributing this, I think it is a good start.

Do you think you could add some basic, simple test? Or do you want me to contribute it? No prob, I can help with that.

if not os.path.isdir(libdir_path):
conanfile.output.verbose(f"{libdir_path} does not exist")
continue
file_count = _flatten_directory(dep, conanfile, libdir_path, output_folder, [".dll", ".dylib",".so"])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we just want to do .dylib and .so, but not .dll? because .dll in Windows go to bindirs, not libdirs (that is the convention)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'am working on the onnxruntime recipe and I noticed that they create dll in the lib directory.
From what I understand they create some "modules" that are supposed to loaded at runtime, and they are considered as "LIBRARY" and not "RUNTIME" by cmake during the installation: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Install.html#installing-targets

Note that according to their documentation the main dll shall be next the to the modules: https://onnxruntime.ai/docs/build/eps.html#loading-the-shared-providers so for this specific recipe we shall move the dll in the bin dir.
What is the correct solution here?

  • Patch the onnxruntime cmake file so that they are copied to the bin folder?
  • In the conanfile:package method, after the cmake install move the dll from the lib to the bin folder?

I am asking in this thread because it shows that a normal cmake install with modules create dll in the lib folder so maybe we have to manage this case in the runtime deployer.

@fdgStilla fdgStilla marked this pull request as draft January 4, 2024 12:56
@fdgStilla
Copy link
Contributor Author

Do you think you could add some basic, simple test? Or do you want me to contribute it? No prob, I can help with that.

I am currently struggling to migrate all our projects to conan 2 and I won't have the time to do work more to this PR so any contribution is more than welcome!

@memsharded
Copy link
Member

Started to add some tests and minor code changes

@Todiq
Copy link

Todiq commented Apr 24, 2024

Hello @memsharded,

Do you have time to keep working on this? I think it could come in very handy for many people. Thanks a lot

@memsharded memsharded added this to the 2.5.0 milestone Jun 10, 2024
@memsharded memsharded marked this pull request as ready for review June 10, 2024 21:51
@memsharded
Copy link
Member

Sorry for the delay, things have been a bit busy, and this was stuck in the backlog.

I have added some missing things:

  • Components support
  • Output scope
  • Avoid unnecessary copies to speed up repeated deploys

Ready to move this for next 2.5

Copy link
Contributor

@czoido czoido left a comment

Choose a reason for hiding this comment

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

I was thinking that maybe as we have now three built-in deployers, it would be nice to list them in the --help, something like this?

  -d DEPLOYER, --deployer DEPLOYER
                        Deploy using the provided deployer to the output folder.
                        Built-in deployers: 'full_deploy', 'direct_deploy', 'runtime_deploy'

@memsharded
Copy link
Member

Done, added docstrings.

@valgur
Copy link
Contributor

valgur commented Jul 3, 2024

I'm kind of surprised this was included in the 2.5.0 release with the two major bugs still present (.so files and symlinks are currently broken, #16527).

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.

[feature] add a runtime deployer
7 participants