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

bdist wheels: exclude build-time-only files #192

Closed
wants to merge 13 commits into from
13 changes: 13 additions & 0 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,19 @@ on:
- bugfix/*
- feature/*
jobs:
sdist:
name: build sdist wheel
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- run: |
make preprocess
python -m pip install --upgrade pip
python -m pip install build setuptools wheel
python -m build --sdist
- uses: actions/upload-artifact@v3
with:
path: ./dist/*.tar.gz
bdist:
name: build bdist wheels and test
runs-on: ${{ matrix.os }}
Expand Down
9 changes: 6 additions & 3 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
include curl_cffi/_wrapper.*
include curl_cffi/ffi/*
include curl_cffi/include/curl/*
recursive-include tests/*
T-256 marked this conversation as resolved.
Show resolved Hide resolved

include ffi/*
include include/curl/*
T-256 marked this conversation as resolved.
Show resolved Hide resolved
include scripts/build.py
include Makefile
T-256 marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 3 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@ curl-impersonate-$(VERSION)/chrome/patches: $(CURL_VERSION)
-o "curl-impersonate-$(VERSION).tar.gz"
tar -xf curl-impersonate-$(VERSION).tar.gz

# TODO add the headers to sdist package
curl_cffi/include/curl/curl.h: curl-impersonate-$(VERSION)/chrome/patches
cd $(CURL_VERSION)
for p in $</curl-*.patch; do patch -p1 < ../$$p; done
# Re-generate the configure script
autoreconf -fi
mkdir -p ../curl_cffi/include/curl
cp -R include/curl/* ../curl_cffi/include/curl/
mkdir -p ../include/curl
cp -R include/curl/* ../include/curl/

preprocess: .preprocessed
@echo preprocess
Expand All @@ -50,6 +49,6 @@ clean:
rm -rf build/ dist/ curl_cffi.egg-info/ $(CURL_VERSION)/ curl-impersonate-$(VERSION)/
rm -rf curl_cffi/*.o curl_cffi/*.so curl_cffi/_wrapper.c
rm -rf .preprocessed .so_downloaded $(CURL_VERSION).tar.xz curl-impersonate-$(VERSION).tar.gz
rm -rf curl_cffi/include/
rm -rf include/

.PHONY: clean build test install-local upload preprocess
53 changes: 0 additions & 53 deletions curl_cffi/build.py

This file was deleted.

File renamed without changes.
File renamed without changes.
File renamed without changes.
4 changes: 0 additions & 4 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ test = [
[tool.setuptools]
packages = ["curl_cffi", "curl_cffi.requests"]
package-data = { curl_cffi = ["libcurl.dll"] }
# include-package-data = true


[build-system]
Expand Down Expand Up @@ -104,9 +103,6 @@ archs = ["auto", "aarch64"]
[tool.cibuildwheel.macos]
before-all = "gmake preprocess"

[tool.isort]
profile = "black"

[tool.ruff.lint]
# Enable the isort rules.
extend-select = ["I"]
Expand Down
79 changes: 79 additions & 0 deletions scripts/build.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import os
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move back this file to its original location for better tracking changes. It should be relocated next time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the purposes of this PR, it tries to exclude all unneeded files from bdist wheels. the changes applied on this file after move is not much, you can compare them easily.

import platform
import struct

from cffi import FFI


def abs_machine():
machine = platform.machine()

pointer_bits = struct.calcsize("P") * 8
if pointer_bits not in (32, 64):
raise Exception("Unsupported pointer size")

is_64 = pointer_bits == 64

# x86 based archs
if machine in ('AMD64', 'x86_64', 'i686'):
return "x86_64" if is_64 else "i686"
# arm based archs
elif machine in ('aarch64', 'arm64', 'armv6l', 'armv7l'):
return "aarch64" if is_64 else "arm"
else:
raise Exception("Unsupported processor")


ffibuilder = FFI()
system = platform.system()
machine = abs_machine()
parent_dir = os.path.dirname(os.path.dirname(__file__))

if system == "Windows":
if machine == "x86_64":
libdir = "./lib32"
elif machine == "i686":
libdir = "./lib64"
else:
libdir = "ERROR"
elif system == "Darwin":
if machine == "x86_64":
libdir = "/Users/runner/work/_temp/install/lib"
else:
libdir = "ERROR"
else:
if machine in ("x86_64", "arm", "aarch64"):
libdir = "/usr/local/lib"
else:
libdir = "ERROR"

if libdir == "ERROR":
raise Exception("Unsupported platform")

ffibuilder.set_source(
"curl_cffi._wrapper",
"""
#include "shim.h"
""",
libraries=["curl-impersonate-chrome"] if system != "Windows" else ["libcurl"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may miss some steps for building windows dll, the name, if correctly configured, should be libcurl-impersonate-chrome.dll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this miss-configuration from curl-impersonate package?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, curl-impersonate, possibly missing autoreconf on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then, IIUC it's not blocker for merge this PR.

library_dirs=[libdir],
source_extension=".c",
include_dirs=[
os.path.join(parent_dir, "include"),
os.path.join(parent_dir, "ffi"),
],
sources=[
os.path.join(parent_dir, "ffi/shim.c"),
],
extra_compile_args=(
["-Wno-implicit-function-declaration"] if system == "Darwin" else []
),
)

with open(os.path.join(parent_dir, "ffi/cdef.c")) as f:
cdef_content = f.read()
ffibuilder.cdef(cdef_content)


if __name__ == "__main__":
ffibuilder.compile(verbose=False)
66 changes: 49 additions & 17 deletions setup.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sys
import os
import platform
import shutil
Expand All @@ -23,40 +24,66 @@ def get_tag(self):
return python, abi, plat


def download_so():
uname = platform.uname()
machine = uname.machine
def abs_machine():
machine = platform.machine()

pointer_bits = struct.calcsize("P") * 8
if pointer_bits not in (32, 64):
raise Exception("Unsupported pointer size")

is_64 = pointer_bits == 64

# x86 based archs
if machine in ('AMD64', 'x86_64', 'i686'):
return "x86_64" if is_64 else "i686"
# arm based archs
elif machine in ('aarch64', 'arm64', 'armv6l', 'armv7l'):
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, I'm not 100% certain if the binary will work on armv6l, since the QEMU emulation targets armv7. I think I have an armv6 raspberry pi 1 somewhere, can test on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw, I'm not 100% certain if the binary will work on armv6l, since the QEMU emulation targets armv7.

sdist supposed to be buildable out of QEMU too.

I think I have an armv6 raspberry pi 1 somewhere, can test on that.

Thanks you.

Copy link
Contributor

Choose a reason for hiding this comment

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

pi@raspberrypi1:~/curl-impersonate $ ./curl-impersonate-chrome
Illegal instruction

I believe armv7 is a superset of the v6 instructions, so v6 will run on v7 but not guaranteed the other way around. If we're interested in v6 compatibility, I can investigate changing the container build pipeline to use v6.

Copy link
Contributor

Choose a reason for hiding this comment

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

After a quick look, building curl-impersonate for armv6 via docker is going to be a massive headache since there are limited base images with armv6 support. It would be easier to run the build on an armv6 raspberry pi, but I would wait and see if there's a need from the community before investing the time and equipment to produce armv6 wheels.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cross compile may also be an option...

Copy link
Contributor

Choose a reason for hiding this comment

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

Trying some options here: bjia56/curl-impersonate#6

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is armv6 still relevant? I think it's only used for RPi 1 and very old android phones, right?

return "aarch64" if is_64 else "arm"
else:
raise Exception("Unsupported processor")


# do not download if target platfrom dll not found
def download_so():
system = platform.system()
machine = abs_machine()

if uname.system == "Windows":
if system == "Windows":
sysname = "win32"
if struct.calcsize("P") * 8 == 64:
machine = "x86_64"
so_name = "libcurl.dll"

if machine == "x86_64":
libdir = "./lib32"
elif machine == "i686":
libdir = "./lib64"
else:
machine = "i686"
libdir = "./lib32"
so_name = "libcurl.dll"
elif uname.system == "Darwin":
so_name = "SKIP"

elif system == "Darwin":
sysname = "macos"
libdir = "/Users/runner/work/_temp/install/lib"
so_name = "libcurl-impersonate-chrome.4.dylib"

if machine == "x86_64":
so_name = "libcurl-impersonate-chrome.4.dylib"
libdir = "/Users/runner/work/_temp/install/lib"
else:
so_name = "SKIP"

else:
sysname = "linux-gnu"
libdir = "/usr/local/lib"
so_name = "libcurl-impersonate-chrome.so"

if machine in ("x86_64", "arm", "aarch64"):
libdir = "/usr/local/lib"
else:
so_name = "SKIP"

if so_name == "SKIP":
print(".so file for platform is not available on github.")
return

if (Path(libdir) / so_name).exists():
print(".so files alreay downloaded.")
return

file = "libcurl-impersonate.tar.gz"
url = (
f"https://github.com/yifeikong/curl-impersonate/releases/download/"
Expand All @@ -70,7 +97,8 @@ def download_so():
print("Unpacking downloaded files...")
os.makedirs(libdir, exist_ok=True)
shutil.unpack_archive(file, libdir)
if uname.system == "Windows":

if system == "Windows":
shutil.copy2(f"{libdir}/libcurl.dll", "curl_cffi")


Expand All @@ -80,11 +108,15 @@ def run(self):
super().run()


# this option is only valid in setup.py
kwargs = {"cffi_modules": ["scripts/build.py:ffibuilder"]}
if len(sys.argv) > 1 and sys.argv[1] != 'bdist_wheel':
kwargs = {}
Comment on lines +113 to +114
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find other way else to detect sdist-only calls


setup(
# this option is only valid in setup.py
cffi_modules=["curl_cffi/build.py:ffibuilder"],
cmdclass={
"bdist_wheel": bdist_wheel_abi3, # type: ignore
"build": my_build, # type: ignore
},
**kwargs,
)