Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix: replace cc_import with cc_library for libpython #820

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

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

scasagrande
Copy link
Contributor

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?

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

Other information

@google-cla
Copy link

google-cla bot commented Sep 2, 2022

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.

@rickeylev
Copy link
Collaborator

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

@scasagrande
Copy link
Contributor Author

scasagrande commented Sep 29, 2022

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

@rickeylev
Copy link
Collaborator

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

@f0rmiga f0rmiga changed the title Replace cc_import with cc_library for libpython fix: replace cc_import with cc_library for libpython Oct 10, 2022
@f0rmiga
Copy link
Member

f0rmiga commented Oct 10, 2022

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 f0rmiga merged commit 85c8186 into bazel-contrib:main Oct 10, 2022
jlaxson added a commit to jlaxson/rules_python that referenced this pull request Nov 19, 2022
jlaxson added a commit to jlaxson/rules_python that referenced this pull request Nov 19, 2022
@scasagrande scasagrande deleted the cc_library_libpython branch February 4, 2025 16:00
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.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.