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.

Sign up for GitHub

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

Add bindings detection to bin targets #938

Merged
merged 1 commit into from
May 28, 2022
Merged

Add bindings detection to bin targets #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Fix python interpreter detection on arm64 Windows in [#940](https://github.com/PyO3/maturin/pull/940)
* Fallback to `py -X.Y` when `pythonX.Y` cannot be found on Windows in [#943](https://github.com/PyO3/maturin/pull/943)
* Auto-detect Python Installs from Microsoft Store in [#944](https://github.com/PyO3/maturin/pull/944)
* Add bindings detection to bin targets in [#938](https://github.com/PyO3/maturin/pull/938)

## [0.12.17] - 2022-05-18

Expand Down
61 changes: 51 additions & 10 deletions src/build_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ pub enum BridgeModel {
/// A native module with c bindings, i.e. `#[no_mangle] extern "C" <some item>`
Cffi,
/// A rust binary to be shipped a python package
Bin,
/// The String is the name of the bindings
/// providing crate, e.g. pyo3, the number is the minimum minor python version
Bin(Option<(String, usize)>),
Copy link
Member Author

Choose a reason for hiding this comment

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

@davidhewitt I wonder should we support abi3 bin bindings, if so I think BridgeModel needs a refactoring to separate bin/lib targets from pyo3/rust-cpython bindings crate. Something like

enum BuildType {
    Bin(Option<BridgeModel>),
    Library(BridgeModel),
}

enum BridgeModel {
    Cffi,
    Bindings(String, usize),
    BindingsAbi3(u8, u8),
}

Copy link
Member

Choose a reason for hiding this comment

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

Based on PyO3/pyo3#2328 (comment) I understood that it's not really possible to link to libpython3.so (i.e. abi3 distribution) without fixing issues in upstream CPython.

/// A native module with pyo3 or rust-cpython bindings. The String is the name of the bindings
/// providing crate, e.g. pyo3, the number is the minimum minor python version
Bindings(String, usize),
Expand All @@ -46,17 +48,24 @@ impl BridgeModel {
/// Test whether this is using a specific bindings crate
pub fn is_bindings(&self, name: &str) -> bool {
match self {
BridgeModel::Bin(Some((value, _))) => value == name,
BridgeModel::Bindings(value, _) => value == name,
_ => false,
}
}

/// Test whether this is bin bindings
pub fn is_bin(&self) -> bool {
matches!(self, BridgeModel::Bin(_))
}
}

impl Display for BridgeModel {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self {
BridgeModel::Cffi => write!(f, "cffi"),
BridgeModel::Bin => write!(f, "bin"),
BridgeModel::Bin(Some((name, _))) => write!(f, "{} bin", name),
BridgeModel::Bin(None) => write!(f, "bin"),
BridgeModel::Bindings(name, _) => write!(f, "{}", name),
BridgeModel::BindingsAbi3(..) => write!(f, "pyo3"),
}
Expand Down Expand Up @@ -213,7 +222,8 @@ impl BuildContext {

let wheels = match &self.bridge {
BridgeModel::Cffi => self.build_cffi_wheel()?,
BridgeModel::Bin => self.build_bin_wheel()?,
BridgeModel::Bin(None) => self.build_bin_wheel(None)?,
BridgeModel::Bin(Some(..)) => self.build_bin_wheels(&self.interpreter)?,
BridgeModel::Bindings(..) => self.build_binding_wheels(&self.interpreter)?,
BridgeModel::BindingsAbi3(major, minor) => {
let cpythons: Vec<_> = self
Expand Down Expand Up @@ -309,7 +319,7 @@ impl BuildContext {
.collect();
others.sort();

if matches!(self.bridge, BridgeModel::Bin) && !musllinux.is_empty() {
if self.bridge.is_bin() && !musllinux.is_empty() {
return get_policy_and_libs(artifact, Some(musllinux[0]), &self.target);
}

Expand Down Expand Up @@ -669,13 +679,22 @@ impl BuildContext {

fn write_bin_wheel(
&self,
python_interpreter: Option<&PythonInterpreter>,
artifact: &Path,
platform_tags: &[PlatformTag],
ext_libs: &[Library],
) -> Result<BuiltWheelMetadata> {
let (tag, tags) = self
.target
.get_universal_tags(platform_tags, self.universal2)?;
let (tag, tags) = match (&self.bridge, python_interpreter) {
(BridgeModel::Bin(None), _) => self
.target
.get_universal_tags(platform_tags, self.universal2)?,
(BridgeModel::Bin(Some(..)), Some(python_interpreter)) => {
let tag =
python_interpreter.get_tag(&self.target, platform_tags, self.universal2)?;
(tag.clone(), vec![tag])
}
_ => unreachable!(),
};

if !self.metadata21.scripts.is_empty() {
bail!("Defining entrypoints and working with a binary doesn't mix well");
Expand Down Expand Up @@ -708,9 +727,12 @@ impl BuildContext {
/// Builds a wheel that contains a binary
///
/// Runs [auditwheel_rs()] if not deactivated
pub fn build_bin_wheel(&self) -> Result<Vec<BuiltWheelMetadata>> {
pub fn build_bin_wheel(
&self,
python_interpreter: Option<&PythonInterpreter>,
) -> Result<Vec<BuiltWheelMetadata>> {
let mut wheels = Vec::new();
let artifacts = compile(self, None, &self.bridge)
let artifacts = compile(self, python_interpreter, &self.bridge)
.context("Failed to build a native library through cargo")?;

let artifact = artifacts
Expand All @@ -725,12 +747,31 @@ impl BuildContext {
self.platform_tag.clone()
};

let (wheel_path, tag) = self.write_bin_wheel(&artifact, &platform_tags, &external_libs)?;
let (wheel_path, tag) = self.write_bin_wheel(
python_interpreter,
&artifact,
&platform_tags,
&external_libs,
)?;
println!("📦 Built wheel to {}", wheel_path.display());
wheels.push((wheel_path, tag));

Ok(wheels)
}

/// Builds a wheel that contains a binary
///
/// Runs [auditwheel_rs()] if not deactivated
pub fn build_bin_wheels(
&self,
interpreters: &[PythonInterpreter],
) -> Result<Vec<BuiltWheelMetadata>> {
let mut wheels = Vec::new();
for python_interpreter in interpreters {
wheels.extend(self.build_bin_wheel(Some(python_interpreter))?);
}
Ok(wheels)
}
}

/// Calculate the sha256 of a file
Expand Down
111 changes: 71 additions & 40 deletions src/build_options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ impl BuildOptions {
}),
)?;

if bridge != BridgeModel::Bin && module_name.contains('-') {
if !bridge.is_bin() && module_name.contains('-') {
bail!(
"The module name must not contains a minus \
(Make sure you have set an appropriate [lib] name in your Cargo.toml)"
Expand Down Expand Up @@ -349,7 +349,7 @@ impl BuildOptions {
}
} else {
// Defaults to musllinux_1_2 for musl target if it's not bin bindings
if target.is_musl_target() && !matches!(bridge, BridgeModel::Bin) {
if target.is_musl_target() && !bridge.is_bin() {
Some(PlatformTag::Musllinux { x: 1, y: 2 })
} else {
None
Expand All @@ -371,8 +371,8 @@ impl BuildOptions {
}

match bridge {
BridgeModel::Bin => {
// Only support two different kind of platform tags when compiling to musl target
BridgeModel::Bin(None) => {
// Only support two different kind of platform tags when compiling to musl target without any binding crates
if platform_tags.iter().any(|tag| tag.is_musllinux()) && !target.is_musl_target() {
bail!(
"Cannot mix musllinux and manylinux platform tags when compiling to {}",
Expand Down Expand Up @@ -550,6 +550,28 @@ fn is_generating_import_lib(cargo_metadata: &Metadata) -> Result<bool> {
Ok(false)
}

/// Tries to determine the bindings type from dependency
fn find_bindings(
deps: &HashMap<&str, &Node>,
packages: &HashMap<&str, &cargo_metadata::Package>,
) -> Option<(String, usize)> {
if deps.get("pyo3").is_some() {
let ver = &packages["pyo3"].version;
let minor =
pyo3_minimum_python_minor_version(ver.major, ver.minor).unwrap_or(MINIMUM_PYTHON_MINOR);
Some(("pyo3".to_string(), minor))
} else if deps.get("pyo3-ffi").is_some() {
let ver = &packages["pyo3-ffi"].version;
let minor = pyo3_ffi_minimum_python_minor_version(ver.major, ver.minor)
.unwrap_or(MINIMUM_PYTHON_MINOR);
Some(("pyo3-ffi".to_string(), minor))
} else if deps.contains_key("cpython") {
Some(("rust-cpython".to_string(), MINIMUM_PYTHON_MINOR))
} else {
None
}
}

/// Tries to determine the [BridgeModel] for the target crate
pub fn find_bridge(cargo_metadata: &Metadata, bridge: Option<&str>) -> Result<BridgeModel> {
let resolve = cargo_metadata
Expand All @@ -574,13 +596,21 @@ pub fn find_bridge(cargo_metadata: &Metadata, bridge: Option<&str>) -> Result<Br
}
})
.collect();
let root_package = cargo_metadata
.root_package()
.context("Expected cargo to return metadata with root_package")?;
let targets: Vec<_> = root_package
.targets
.iter()
.flat_map(|target| target.crate_types.iter())
.map(String::as_str)
.collect();

let bridge = if let Some(bindings) = bridge {
if bindings == "cffi" {
BridgeModel::Cffi
} else if bindings == "bin" {
println!("🔗 Found bin bindings");
BridgeModel::Bin
BridgeModel::Bin(find_bindings(&deps, &packages))
} else {
if !deps.contains_key(bindings) {
bail!(
Expand All @@ -591,41 +621,26 @@ pub fn find_bridge(cargo_metadata: &Metadata, bridge: Option<&str>) -> Result<Br

BridgeModel::Bindings(bindings.to_string(), MINIMUM_PYTHON_MINOR)
}
} else if deps.get("pyo3").is_some() {
let ver = &packages["pyo3"].version;
let minor =
pyo3_minimum_python_minor_version(ver.major, ver.minor).unwrap_or(MINIMUM_PYTHON_MINOR);
BridgeModel::Bindings("pyo3".to_string(), minor)
} else if deps.get("pyo3-ffi").is_some() {
let ver = &packages["pyo3-ffi"].version;
let minor = pyo3_ffi_minimum_python_minor_version(ver.major, ver.minor)
.unwrap_or(MINIMUM_PYTHON_MINOR);
BridgeModel::Bindings("pyo3-ffi".to_string(), minor)
} else if deps.contains_key("cpython") {
println!("🔗 Found rust-cpython bindings");
BridgeModel::Bindings("rust_cpython".to_string(), MINIMUM_PYTHON_MINOR)
} else {
let package = cargo_metadata
.root_package()
.context("Expected cargo to return metadata with root_package")?;
let targets: Vec<_> = package
.targets
.iter()
.flat_map(|target| target.crate_types.iter())
.map(String::as_str)
.collect();

if targets.contains(&"cdylib") {
BridgeModel::Cffi
} else if targets.contains(&"bin") {
BridgeModel::Bin
} else if let Some((bindings, minor)) = find_bindings(&deps, &packages) {
if !targets.contains(&"cdylib") && targets.contains(&"bin") {
BridgeModel::Bin(Some((bindings, minor)))
} else {
bail!("Couldn't detect the binding type; Please specify them with --bindings/-b")
BridgeModel::Bindings(bindings, minor)
}
} else if targets.contains(&"cdylib") {
BridgeModel::Cffi
} else if targets.contains(&"bin") {
BridgeModel::Bin(find_bindings(&deps, &packages))
} else {
bail!("Couldn't detect the binding type; Please specify them with --bindings/-b")
};

if !(bridge.is_bindings("pyo3") || bridge.is_bindings("pyo3-ffi")) {
println!("🔗 Found {} bindings", bridge);
}

for &lib in PYO3_BINDING_CRATES.iter() {
if bridge.is_bindings(lib) {
if !bridge.is_bin() && bridge.is_bindings(lib) {
let pyo3_node = deps[lib];
if !pyo3_node.features.contains(&"extension-module".to_string()) {
let version = cargo_metadata[&pyo3_node.id].version.to_string();
Expand Down Expand Up @@ -771,7 +786,7 @@ pub fn find_interpreter(
generate_import_lib: bool,
) -> Result<Vec<PythonInterpreter>> {
match bridge {
BridgeModel::Bindings(binding_name, _) => {
BridgeModel::Bindings(binding_name, _) | BridgeModel::Bin(Some((binding_name, _))) => {
let mut native_interpreters = false;
let mut interpreters = Vec::new();
if let Some(config_file) = env::var_os("PYO3_CONFIG_FILE") {
Expand Down Expand Up @@ -893,7 +908,7 @@ pub fn find_interpreter(
println!("🐍 Using {} to generate the cffi bindings", interpreter);
Ok(vec![interpreter])
}
BridgeModel::Bin => Ok(vec![]),
BridgeModel::Bin(None) => Ok(vec![]),
BridgeModel::BindingsAbi3(major, minor) => {
let interpreter = if !interpreter.is_empty() {
PythonInterpreter::check_executables(interpreter, target, bridge)
Expand Down Expand Up @@ -1125,12 +1140,28 @@ mod test {

assert_eq!(
find_bridge(&hello_world, Some("bin")).unwrap(),
BridgeModel::Bin
BridgeModel::Bin(None)
);
assert_eq!(
find_bridge(&hello_world, None).unwrap(),
BridgeModel::Bin(None)
);
assert_eq!(find_bridge(&hello_world, None).unwrap(), BridgeModel::Bin);

assert!(find_bridge(&hello_world, Some("rust-cpython")).is_err());
assert!(find_bridge(&hello_world, Some("pyo3")).is_err());

let pyo3_bin = MetadataCommand::new()
.manifest_path(&Path::new("test-crates/pyo3-bin").join("Cargo.toml"))
.exec()
.unwrap();
assert!(matches!(
find_bridge(&pyo3_bin, Some("bin")).unwrap(),
BridgeModel::Bin(Some((..)))
));
assert!(matches!(
find_bridge(&pyo3_bin, None).unwrap(),
BridgeModel::Bin(Some(..))
));
}

#[test]
Expand Down
9 changes: 5 additions & 4 deletions src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ fn compile_universal2(
python_interpreter: Option<&PythonInterpreter>,
bindings_crate: &BridgeModel,
) -> Result<HashMap<String, PathBuf>> {
let build_type = match bindings_crate {
BridgeModel::Bin => "bin",
_ => "cdylib",
let build_type = if bindings_crate.is_bin() {
"bin"
} else {
"cdylib"
};
let mut aarch64_context = context.clone();
aarch64_context.cargo_extra_args.extend(vec![
Expand Down Expand Up @@ -137,7 +138,7 @@ fn compile_target(
// We need to pass --bins / --lib to set the rustc extra args later
// TODO: What do we do when there are multiple bin targets?
match bindings_crate {
BridgeModel::Bin => shared_args.push("--bins"),
BridgeModel::Bin(..) => shared_args.push("--bins"),
BridgeModel::Cffi | BridgeModel::Bindings(..) | BridgeModel::BindingsAbi3(..) => {
shared_args.push("--lib");
// https://github.com/rust-lang/rust/issues/59302#issue-422994250
Expand Down
4 changes: 2 additions & 2 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ fn pep517(subcommand: Pep517Command) -> Result<()> {

// Since afaik all other PEP 517 backends also return linux tagged wheels, we do so too
let tags = match context.bridge {
BridgeModel::Bindings(..) => {
BridgeModel::Bindings(..) | BridgeModel::Bin(Some(..)) => {
vec![context.interpreter[0].get_tag(
&context.target,
&[PlatformTag::Linux],
Expand All @@ -274,7 +274,7 @@ fn pep517(subcommand: Pep517Command) -> Result<()> {
.get_platform_tag(&[PlatformTag::Linux], context.universal2)?;
vec![format!("cp{}{}-abi3-{}", major, minor, platform)]
}
BridgeModel::Bin | BridgeModel::Cffi => {
BridgeModel::Bin(None) | BridgeModel::Cffi => {
context
.target
.get_universal_tags(&[PlatformTag::Linux], context.universal2)?
Expand Down
Loading