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

[libffi] Use upstream's build system #33203

Merged
merged 14 commits into from
Aug 24, 2023
Merged

[libffi] Use upstream's build system #33203

merged 14 commits into from
Aug 24, 2023

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Aug 15, 2023

Uses the official build system.
Uses the official binary names for Windows. (One patch less for llvm.)
Reimplements exported CMake config with find_... commands, and moves it to the unofficial namespace. (Migration path included.)

Adds libffi dependency to gobject-introspection manifest.

@LilyWangLL LilyWangLL self-assigned this Aug 16, 2023
@LilyWangLL LilyWangLL added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Aug 16, 2023
@dg0yt dg0yt mentioned this pull request Aug 16, 2023
7 tasks
@dg0yt dg0yt marked this pull request as ready for review August 18, 2023 05:44
ports/libffi/usage Outdated Show resolved Hide resolved
@LilyWangLL
Copy link
Contributor

Can you please resolve the conflicts against master?

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 22, 2023

Just the usual merge-unfriendly vcpkg versions stuff. Done.

@LilyWangLL
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dg0yt dg0yt requested a review from LilyWangLL August 24, 2023 05:45
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Aug 24, 2023
@dan-shaw dan-shaw merged commit edd3db2 into microsoft:master Aug 24, 2023
15 checks passed
@dg0yt dg0yt deleted the libffi branch August 24, 2023 20:04
@TrueBrain
Copy link
Contributor

This change seems to break Python3 on x64-linux. It installs libffi in lib64 folder, instead of lib folder. This causes Python to not link against libffi, causing unresolved symbol errors. For more information: #33426

@Neumann-A
Copy link
Contributor

This change seems to break Python3 on x64-linux. It installs libffi in lib64 folder, instead of lib folder.

What linux distro is that? the one vcpkg uses installs into /lib/

@TrueBrain
Copy link
Contributor

As mentioned in #33426, this is with quay.io/pypa/manylinux2014_x86_64 container image. Which is used to build a lot of native Python libraries, as it is lovely compatible with a lot of different Linux machines.

@Neumann-A
Copy link
Contributor

Try passing --disable-multi-os-directory to vcpkg_configure_make in the portfile. That is the only hint I currently have why the build might be ignoring the libdir setting of vcpkg.

@TrueBrain
Copy link
Contributor

diff --git a/ports/libffi/portfile.cmake b/ports/libffi/portfile.cmake
index eaef8c8..40d936b 100644
--- a/ports/libffi/portfile.cmake
+++ b/ports/libffi/portfile.cmake
@@ -48,6 +48,7 @@ vcpkg_configure_make(
     OPTIONS
         --enable-portable-binary
         --disable-docs
+        --disable-multi-os-directory
         ${options}
 )

With that patch, it installs and works correctly. Shall I make a PR for this? (as in, is that an acceptable solution, or does it need more love somewhere)

@Neumann-A
Copy link
Contributor

With that patch, it installs and works correctly. Shall I make a PR for this?

For me it looks like the correct solution so why not?

@Flole998
Copy link
Contributor

Flole998 commented May 19, 2024

It looks like this broke the builds on x86-linux, at least when building on a x64 linux system. It compiles just fine, however it is failing to add x86/ffi.lo to the target objects so that file is not compiled and linked into the final .a file, which then leads to ffi_call and other important functions to be missing. I've tried to hack around a little to specify the target passed to configure manually, but that didn't help either. I'm out of ideas now on how to convince it to properly build it.

It looks like -m32 only ends up in CXXFLAGS and not in CFLAGS for some reason. That causes the detection to not work properly anymore.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 20, 2024

Please open a proper issue. The templates should remind you of neccessary information (platform, logs, etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants