facebook / sapling

A Scalable, User-Friendly Source Control System.
https://sapling-scm.com
GNU General Public License v2.0
6.1k stars 279 forks source link

hgmain: fix undefined python symbol at runtime. #896

Closed thb-sb closed 4 months ago

thb-sb commented 4 months ago

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, 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/python@3.11/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 commented 4 months ago

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 cla@meta.com. Thanks!

facebook-github-bot commented 4 months ago

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

thb-sb commented 4 months ago

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.

https://github.com/facebook/sapling/blob/6c49fe382c91bc1a28be0a6c3d348b159a96c957/eden/scm/saplingnative/bindings/modules/pygitcompat/Cargo.toml#L1

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

muirdm commented 4 months ago

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

facebook-github-bot commented 4 months ago

@muirdm merged this pull request in facebook/sapling@01313b573e830619dfbc76c729b5c2b91f8c4569.