-
-
Notifications
You must be signed in to change notification settings - Fork 593
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
Conversation
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. |
fcc4d74
to
515e51e
Compare
@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. |
I just came up with another solution for myself that completely avoids having I enabled the |
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. |
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. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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 includelibpythonX.Y.so
via depending on@python//:libpython
.Does this PR introduce a breaking change?
Other information