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

GH-127970: find the runtime library when dladdr is available #127972

Merged
merged 13 commits into from
Jan 8, 2025

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Dec 15, 2024

Signed-off-by: Filipe Laíns <[email protected]
@FFY00
Copy link
Member Author

FFY00 commented Dec 15, 2024

If this gets backported, GH-127974 should too, to ensure we don't end up with mismatched prefixes from different installations.

@FFY00 FFY00 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 15, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @FFY00 for commit 5230b61 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 15, 2024
@FFY00 FFY00 removed needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Dec 17, 2024
@FFY00
Copy link
Member Author

FFY00 commented Dec 17, 2024

@edelsohn, @ayappanec, I was unable to find much concrete information regarding the dladdr availability on AIX? I see some references to it not being available, like boostorg/stacktrace#89, but the 2022 documentation, the oldest available on the IBM website, mention it being available. Do you know which AIX version added dladdr, so that I can it that to our documentation? Thanks!

@ayappanec
Copy link
Contributor

AIX don't have dladdr. The IBM documentation is about z/TPF operating system ( and not AIX).

@FFY00
Copy link
Member Author

FFY00 commented Dec 17, 2024

Ah, gotcha! Thanks!

Signed-off-by: Filipe Laíns <[email protected]
Signed-off-by: Filipe Laíns <[email protected]
@FFY00
Copy link
Member Author

FFY00 commented Dec 17, 2024

I tested this on macOS in framework and non-framework builds, and seems to work fine. I'll give it a couple more days for other to have time to review.

@ned-deily ned-deily self-requested a review December 22, 2024 07:34
@ned-deily
Copy link
Member

My apologies for the delay in testing.

The PR as it stands appears to break installed macOS frameworks builds, the standard configuration used by many downstream distributors including python.org installers. To reproduce without doing a system-wide install:

cd cpython
./configure --enable-framework=$HOME/test_127970/Library/Frameworks
make -j4 && make install
cd
$HOME/test_127970/bin/python3.14

Resulting in:

Exception ignored in running getpath:
Traceback (most recent call last):
  File "<frozen getpath>", line 313, in <module>
TypeError: argument 1 must be str, not None
Fatal Python error: error evaluating path
Python runtime state: core initialized

Current thread 0x00000001f5598240 (most recent call first):
  <no Python frame>

@FFY00
Copy link
Member Author

FFY00 commented Jan 7, 2025

Thank you for the feedback, I'll have a look!

Signed-off-by: Filipe Laíns <[email protected]
@FFY00
Copy link
Member Author

FFY00 commented Jan 7, 2025

Okay, this was a bit tricky to debug, but it seems like the path variable was somehow being optimized out and messing things up. Simply changing from -O3 to -Og causes the issue to go away, for eg. That's why I didn't catch the issue previously, I tested with --with-pydebug. To solve the issue I just went away with the variable, and instead return directly from Dl_info.

Could you confirm that it fixes the issue on your side?

@ned-deily
Copy link
Member

Could you confirm that it fixes the issue on your side?

Yes, thanks, that does solve the build problem.

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

With the build fix, I did builds and tests on current macOS versions of all three install variants - default (non-shared), shared, and framework - and noted no regressions. I also did a framework build and test on an older (macos 10.11) system with no apparent regression. Unfortunately, I didn't have available an applicable test of embedding Python on macOS. And I did not do any testing on other (non-macOS) platforms including iOS nor did I do any additional testing of venv's outside of the standard test suite. I didn't note any obvious red flags in the getpath.c changes but I'm not an expert in the details of interpreter startup across the various platforms. As such, I'm +0 on making this change; if we want to do it, we should do it soon, that is, early in the release cycle to get exposure.

@FFY00
Copy link
Member Author

FFY00 commented Jan 8, 2025

Thanks for the feedback, I am gonna go ahead and sync the branch with main to solve the generated files CI failure, and merge afterwards. If we notice any issues, we can come back and re-consider, reverting if necessary.

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.

4 participants