Skip to content

Commit

Permalink
Warn about missing PyInit function to fix #124
Browse files Browse the repository at this point in the history
  • Loading branch information
konstin committed May 12, 2019
1 parent 920ce16 commit fe585b1
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 6 deletions.
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ regex = "1.1.6"
indicatif = "0.11.0"
atty = "0.2.11"
tempfile = "3.0.7"
goblin = { version = "0.0.22", optional = true }
goblin = "0.0.22"
pretty_env_logger = { version = "0.3.0", optional = true }
platforms = "0.2.0"
shlex = "0.1.1"
Expand All @@ -52,7 +52,7 @@ indoc = "0.3.3"

[features]
default = ["auditwheel", "log", "upload", "rustls"]
auditwheel = ["goblin"]
auditwheel = []
upload = ["reqwest", "rpassword"]
password-storage = ["upload", "keyring"]
log = ["pretty_env_logger"]
Expand Down
14 changes: 12 additions & 2 deletions src/build_context.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#[cfg(feature = "auditwheel")]
use crate::auditwheel::auditwheel_rs;
use crate::compile;
use crate::compile::warn_missing_py_init;
use crate::module_writer::WheelWriter;
use crate::module_writer::{write_bin, write_bindings_module, write_cffi_module};
use crate::Manylinux;
Expand Down Expand Up @@ -100,7 +101,7 @@ impl BuildContext {
) -> Result<Vec<(PathBuf, String, Option<PythonInterpreter>)>, Error> {
let mut wheels = Vec::new();
for python_version in &self.interpreter {
let artifact = self.compile_cdylib(Some(&python_version))?;
let artifact = self.compile_cdylib(Some(&python_version), Some(&self.module_name))?;

let tag = python_version.get_tag(&self.manylinux);

Expand Down Expand Up @@ -137,9 +138,13 @@ impl BuildContext {

/// Runs cargo build, extracts the cdylib from the output, runs auditwheel and returns the
/// artifact
///
/// The module name is used to warn about missing a `PyInit_<module name>` function for
/// bindings modules.
pub fn compile_cdylib(
&self,
python_interpreter: Option<&PythonInterpreter>,
module_name: Option<&str>,
) -> Result<PathBuf, Error> {
let artifacts = compile(&self, python_interpreter, &self.bridge)
.context("Failed to build a native library through cargo")?;
Expand All @@ -159,6 +164,11 @@ impl BuildContext {
auditwheel_rs(&artifact, target, &self.manylinux)
.context("Failed to ensure manylinux compliance")?;

if let Some(module_name) = module_name {
warn_missing_py_init(&artifact, module_name)
.context("Failed to parse the native library")?;
}

Ok(artifact)
}

Expand All @@ -173,7 +183,7 @@ impl BuildContext {

/// Builds a wheel with cffi bindings
pub fn build_cffi_wheel(&self) -> Result<PathBuf, Error> {
let artifact = self.compile_cdylib(None)?;
let artifact = self.compile_cdylib(None, None)?;

let (tag, tags) = self.get_unversal_tags();

Expand Down
42 changes: 42 additions & 0 deletions src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ use indicatif::{ProgressBar, ProgressStyle};
use serde::Deserialize;
use serde_json;
use std::collections::HashMap;
use std::fs::File;
use std::io::Read;
use std::path::PathBuf;
use std::process::{Command, Stdio};
use std::str;
Expand Down Expand Up @@ -273,3 +275,43 @@ pub fn compile(

Ok(artifacts)
}

/// Checks that the native library contains a function called `PyInit_<module name>` and warns
/// if it's missing.
///
/// That function is the python's entrypoint for loading native extensions, i.e. python will fail
/// to import the module with error if it's missing or named incorrectly
///
/// Currently the check is only run on linux
pub fn warn_missing_py_init(artifact: &PathBuf, module_name: &str) -> Result<(), Error> {
let py_init = format!("PyInit_{}", module_name);
let mut fd = File::open(&artifact)?;
let mut buffer = Vec::new();
fd.read_to_end(&mut buffer)?;
let mut found = false;
match goblin::Object::parse(&buffer)? {
goblin::Object::Elf(elf) => {
for dyn_sym in elf.dynsyms.iter() {
if py_init == elf.dynstrtab[dyn_sym.st_name] {
found = true;
break;
}
}
}
_ => {
// Currently, only linux is implemented
found = true
}
}

if !found {
println!(
"⚠ Warning: Couldn't find the symbol `{}` in the native library. \
Python will fail to import this module. \
If you're using pyo3, check that `#[pymodule]` uses `{}` as module name",
py_init, module_name
)
}

Ok(())
}
4 changes: 2 additions & 2 deletions src/develop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub fn develop(
fs::copy(artifact, bin_path)?;
}
BridgeModel::Cffi => {
let artifact = build_context.compile_cdylib(None).context(context)?;
let artifact = build_context.compile_cdylib(None, None).context(context)?;

write_cffi_module(
&mut builder,
Expand All @@ -77,7 +77,7 @@ pub fn develop(
}
BridgeModel::Bindings(_) => {
let artifact = build_context
.compile_cdylib(Some(&interpreter))
.compile_cdylib(Some(&interpreter), Some(&build_context.module_name))
.context(context)?;

write_bindings_module(
Expand Down

0 comments on commit fe585b1

Please sign in to comment.