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

[python3] x64-linux-dynamic fixes #25995

Merged
merged 4 commits into from
Aug 4, 2022

Conversation

Osyotr
Copy link
Contributor

@Osyotr Osyotr commented Jul 26, 2022

This PR fixes build issues with x64-linux-dynamic triplet.

  1. Patch setup.py to force rpath flags for extensions.
  2. Implement some post-build checks to ensure extensions built successfully.
  3. Set VCPKG_POLICY_MISMATCHED_NUMBER_OF_BINARIES because there's an additional binary in release builds (see comment in portfile).
  4. Add missing libuuid dependency

For #25668

github-actions[bot]
github-actions bot previously approved these changes Jul 26, 2022
@Osyotr Osyotr changed the title [python3] x64-linux-debug fixes [python3] x64-linux-dynamic fixes Jul 26, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 26, 2022
@Adela0814 Adela0814 added the category:port-bug The issue is with a library, which is something the port should already support label Jul 27, 2022
Adela0814
Adela0814 previously approved these changes Jul 27, 2022
@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Jul 27, 2022
@Adela0814 Adela0814 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 27, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 27, 2022
@Osyotr
Copy link
Contributor Author

Osyotr commented Jul 27, 2022

@Hoikas is it worth adding VCPKG_POLICY_MISMATCHED_NUMBER_OF_BINARIES just to keep python stable abi library (libpython3.so)?

@Hoikas
Copy link
Contributor

Hoikas commented Jul 27, 2022

I'm not sure how widely the limited API library is used, TBH. If you have a use for it or it's trivial to make available, I think it would be a good thing to have as an option.

@Osyotr
Copy link
Contributor Author

Osyotr commented Jul 28, 2022

It's already available but with the cost of VCPKG_POLICY_MISMATCHED_NUMBER_OF_BINARIES :(
I don't use it myself and haven't found any projects that actually use it. Both boost-python and SWIG haven't adopted it yet.
boostorg/python#221
swig/swig#2190

@Osyotr
Copy link
Contributor Author

Osyotr commented Aug 1, 2022

@Adela0814 maybe vcpkg-team-review this?

Adela0814
Adela0814 previously approved these changes Aug 2, 2022
@Adela0814 Adela0814 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 2, 2022
@@ -204,6 +205,11 @@ if(VCPKG_TARGET_IS_WINDOWS OR VCPKG_TARGET_IS_UWP)
endif()
endif()
else()
# Python Stable ABI is incompatible with --with-pydebug option
Copy link
Member

Choose a reason for hiding this comment

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

vcpkg does not provide a stable ABI for installed trees, so I'm not sure why this is a relevant distinction? Note that vcpkg's python almost never matches the system python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what their buildsystem does.
Upon further investigation, I found out that debug builds are now abi compatible with release builds python/cpython#12615
Maybe ask maintainers whether this is something they forgot to address in their buildsystem?

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Set VCPKG_POLICY_MISMATCHED_NUMBER_OF_BINARIES because there's an additional binary in release builds (see comment in portfile).

Can you explain what the additional binary is?

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 3, 2022
@Osyotr
Copy link
Contributor Author

Osyotr commented Aug 4, 2022

@BillyONeal the additional binary is libpython3.so aka "Python Stable ABI". It's not created in debug builds.
https://peps.python.org/pep-0384/#linkage

@BillyONeal
Copy link
Member

@BillyONeal the additional binary is libpython3.so aka "Python Stable ABI". It's not created in debug builds. https://peps.python.org/pep-0384/#linkage

Ah, I see. "Python Stable ABI" isn't a statement about the shipping Python bits, it's the name of a specific binary.

@BillyONeal BillyONeal added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 4, 2022
@BillyONeal BillyONeal merged commit e221326 into microsoft:master Aug 4, 2022
@BillyONeal
Copy link
Member

Thanks!

self.add(Extension('readline', ['readline.c'],
library_dirs=['/usr/lib/termcap'],
extra_link_args=readline_extra_link_args,
+ runtime_library_dirs=self.lib_dirs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from using absolute paths, this patch broke the static build. The build ends successful, but it lacks important components such as binascii. The error logs lists unknown -R... parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use of absolute paths is ok at build time. They will be fixed when installing.
I'll try to get rid of this patch later today and disable python's check for whether module loads or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants