rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

Replace cc_import with cc_library for libpython

Open scasagrande opened this issue 3 years ago • 1 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [ ] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [x] Bugfix
  • [ ] Feature (please, look at the "Scope of the project" section in the README.md file)
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Documentation content changes
  • [ ] Other... Please describe:

What is the current behavior?

Issue Number: #796

What is the new behavior?

This allows rust library targets that utilize pyo3 to correctly be able to include libpythonX.Y.so via depending on @python//:libpython.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

scasagrande avatar Sep 02 '22 21:09 scasagrande

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Sep 02 '22 21:09 google-cla[bot]

@f0rmiga would you be willing/able to have a look? From the associated issue, it sounds like you would be better qualified to judge.

At a glance, I would have expected cc_import to Just Work, since it's doc indicates it's for prebuilt shared libraries, which would imply either (a) something downstream of it is incorrect, or (b) something is missing in our cc_import() call. But I don't know enough about the difference between cc_import and cc_library (and the lower-level linker stuff mentioned in the issue) to say with any certainty.

rickeylev avatar Sep 28 '22 20:09 rickeylev

I just came up with another solution for myself that completely avoids having libpython as a dependency for my pyo3 target. I should have thought of this ahead of time, as this is important for manylinux compliance.

I enabled the "extension-module" and "abi3-py38" features for pyo3 and now I don't need libpython as a dependency

scasagrande avatar Sep 29 '22 14:09 scasagrande

I just came up with another solution for myself that completely avoids having libpython as a dependency for my pyo3 target. I should have thought of this ahead of time, as this is important for manylinux compliance.

Great, does that mean changing it to cc_library is unnecessary now? If so, if you want to pivot this PR to adding a comment, e.g. # NOTE: if you're depending this from rust, then ..., we can finish review. It sounds like a good hint to give future people trying to do rust+python interop

rickeylev avatar Sep 29 '22 22:09 rickeylev

I still think the approach in the PR is correct. In practical terms, the difference between cc_library and cc_import is that cc_import only takes 1 file, so in the case of Linux that has 2 files (the real shared object and the symlink to it), there's no way to use cc_import. For one case I worked on, the cc_import was fine because we were using RBE, and since RBE converts the symlinks into the realpath contents, it worked fine.

f0rmiga avatar Oct 10 '22 01:10 f0rmiga