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

hgmain: fix undefined python symbol at runtime. #896

Closed
wants to merge 1 commit into from
Closed

hgmain: fix undefined python symbol at runtime. #896

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 15, 2024

Commit dc45ccb introduced pygitcompat to talk to git from python.

extension-module is enabled:

cpython = { version = "0.7.1", features = ["extension-module", "python3-sys", "serde-convert"], default-features = false }

which ultimately enables python3-sys/extension-module.

According to the python3-sys documentation, extension-module prevents the final artifact from being linked against the shared Python library:

 # Use this feature when building an extension module.
 # It tells the linker to keep the python symbols unresolved,
 # so that the module can also be used with statically linked python interpreters.
extension-module = [ "python3-sys/extension-module" ]

It leads to a bug where the final binary sl is not linked against Python:

$ otool -L /tmp/sl_dc45ccb
/tmp/sl_dc45ccb:
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1700.255.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
        /System/Library/Frameworks/Cocoa.framework/Versions/A/Cocoa (compatibility version 1.0.0, current version 24.0.0)
        /System/Library/Frameworks/WebKit.framework/Versions/A/WebKit (compatibility version 1.0.0, current version 618.1.15)
        /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
        /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 61123.100.169)
        /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2420.0.0)
        /opt/homebrew/opt/openssl@3/lib/libssl.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1226.0.0)
        /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration (compatibility version 1.0.0, current version 1300.100.9)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
$ /tmp/sl_dc45ccb
dyld[8926]: symbol not found in flat namespace '_PyBool_Type'
Abort trap: 6

This commit removes the extension-module feature flag to link again against libpython:

$ otool -L /tmp/sl_fix
/tmp/sl_fix:
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1700.255.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
        /System/Library/Frameworks/Cocoa.framework/Versions/A/Cocoa (compatibility version 1.0.0, current version 24.0.0)
        /System/Library/Frameworks/WebKit.framework/Versions/A/WebKit (compatibility version 1.0.0, current version 618.1.15)
        /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
        /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 61123.100.169)
        /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2420.0.0)
        /opt/homebrew/opt/openssl@3/lib/libssl.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1226.0.0)
        /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration (compatibility version 1.0.0, current version 1300.100.9)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
>>>>>   /opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/Python (compatibility version 3.11.0, current version 3.11.0)
        /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)

@facebook-github-bot
Copy link
Contributor

Hi @thb-sb!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

Commit [`dc45ccb`] introduced `pygitcompat` to talk to git from python.

`extension-module` is enabled:

https://github.com/facebook/sapling/blob/dc45ccbbc2bafb850e4535159d6c96793f27738e/eden/scm/saplingnative/bindings/modules/pygitcompat/Cargo.toml#L10

which ultimately enables `python3-sys/extension-module`.

According to [the `python3-sys` documentation](https://github.com/dgrunwald/rust-cpython/blob/e815555629e557be084813045ca1ddebc2f76ef9/Cargo.toml#L71-L74), `extension-module` prevents the final artifact
from being linked against the shared Python library:

```toml
 # Use this feature when building an extension module.
 # It tells the linker to keep the python symbols unresolved,
 # so that the module can also be used with statically linked python interpreters.
extension-module = [ "python3-sys/extension-module" ]
```

It leads to a bug where the final binary `sl` is not linked against Python:

```
$ otool -L /tmp/sl_dc45ccb
/tmp/sl_dc45ccb:
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1700.255.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
        /System/Library/Frameworks/Cocoa.framework/Versions/A/Cocoa (compatibility version 1.0.0, current version 24.0.0)
        /System/Library/Frameworks/WebKit.framework/Versions/A/WebKit (compatibility version 1.0.0, current version 618.1.15)
        /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
        /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 61123.100.169)
        /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2420.0.0)
        /opt/homebrew/opt/openssl@3/lib/libssl.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1226.0.0)
        /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration (compatibility version 1.0.0, current version 1300.100.9)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
        /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
$ /tmp/sl_dc45ccb
dyld[8926]: symbol not found in flat namespace '_PyBool_Type'
Abort trap: 6
```

This commit removes the `extension-module` feature flag to link again against
libpython:

```
$ otool -L /tmp/sl_fix
/tmp/sl_fix:
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1700.255.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
        /System/Library/Frameworks/Cocoa.framework/Versions/A/Cocoa (compatibility version 1.0.0, current version 24.0.0)
        /System/Library/Frameworks/WebKit.framework/Versions/A/WebKit (compatibility version 1.0.0, current version 618.1.15)
        /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
        /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 61123.100.169)
        /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2420.0.0)
        /opt/homebrew/opt/openssl@3/lib/libssl.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /opt/homebrew/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
        /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1226.0.0)
        /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration (compatibility version 1.0.0, current version 1300.100.9)
        /usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.12)
>>>>>   /opt/homebrew/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/Python (compatibility version 3.11.0, current version 3.11.0)
        /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
```

[`dc45ccb`]: dc45ccb
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ghost ghost marked this pull request as ready for review May 15, 2024 10:33
@ghost
Copy link
Author

ghost commented May 15, 2024

Hi sapling team! 👋
I've seen that the file I've modified is generated by some internal tool you have (I guess autocargo?), but I couldn't find the original files used by that generator.

# @generated by autocargo from //eden/scm/saplingnative/bindings/modules/pygitcompat:pygitcompat

If you can provide me with some hints I could make it right!

@muirdm
Copy link
Contributor

muirdm commented May 15, 2024

I've imported your PR and turned the features off in the underlying build file. Thanks!

@facebook-github-bot
Copy link
Contributor

@muirdm merged this pull request in 01313b5.

@ghost ghost deleted the pr894 branch May 15, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants