Proper PEP 517 sdist support (#2)

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 authors = ["konstin <konstin@mailbox.org>"]
 name = "pyo3-pack"
-version = "0.6.1"
+version = "0.7.0"
 description = "Build and publish crates with pyo3, rust-cpython and cffi bindings as well as rust binaries as python packages"
 exclude = ["test-crates/**/*", "integration-test/**/*", "sysconfig/*", "test-data/*"]
 readme = "Readme.md"

Readme.md

@@ -118,14 +118,16 @@ my-project
 
 ## pyproject.toml
 
-pyo3-pack has partial support for building through pyproject.toml. To use it, create a `pyproject.toml` next to your `Cargo.toml` with the following content:
+pyo3-pack supports building through pyproject.toml. To use it, create a `pyproject.toml` next to your `Cargo.toml` with the following content:
 
 ```toml
 [build-system]
 requires = ["pyo3-pack", "toml"]
 build-backend = "pyo3_pack"
 ```
 
+If a `pyproject.toml` with a `[build-system]` entry is present, pyo3-pack will build a source distribution (sdist) of your package, unless `--no-sdist` is specified.
+
 You can then e.g. install your package with `pip install .`. With `pip install . -v` you can see the output of cargo and pyo3-pack.
 
 You can use the options `manylinux`, `skip-auditwheel`, `bindings`, `cargo-extra-args` and `rustc-extra-args` under `[tool.pyo3-pack]` the same way you would when running pyo3-pack directly. The `bindings` key is required for cffi and bin projects as those can't be automatically detected.
@@ -140,7 +142,7 @@ bindings = "cffi"
 cargo-extra-args="-v"
 ```
 
-Currently, only the wheel build part of [PEP 517](https://snarky.ca/clarifying-pep-518/) is supported. Building source distribution doesn't work yet, which means isolated builds will fail.
+Using tox with build isolation is currently blocked by a tox bug ([tox-dev/tox#1344](https://github.com/tox-dev/tox/issues/1344)).
 
 ## Manylinux and auditwheel
 

pyo3_pack/__init__.py

@@ -11,6 +11,7 @@
 import os
 import shutil
 import subprocess
+import sys
 from typing import List, Dict
 
 import toml
@@ -46,11 +47,16 @@ def get_config_options() -> List[str]:
 
 # noinspection PyUnusedLocal
 def build_wheel(wheel_directory, config_settings=None, metadata_directory=None):
-    command = ["pyo3-pack", "build", "-i", "python"]
+    # The PEP 517 build environment garuantees that `python` is the correct python
+    command = ["pyo3-pack", "build", "-i", "python", "--no-sdist"]
     command.extend(get_config_options())
 
     print("Running `{}`".format(" ".join(command)))
-    output = subprocess.check_output(command, universal_newlines=True)
+    try:
+        output = subprocess.check_output(command, universal_newlines=True)
+    except subprocess.CalledProcessError as e:
+        print("Error: {}".format(e))
+        sys.exit(1)
     print(output)
     # Get the filename from `📦 Built wheel [for CPython 3.6m] to /home/user/project`
     filename = os.path.split(output.strip().splitlines()[-1].split(" to ")[1])[1]
@@ -70,7 +76,11 @@ def build_sdist(sdist_directory, config_settings=None):
     command.extend(get_config_options())
 
     print("Running `{}`".format(" ".join(command)))
-    output = subprocess.check_output(command, universal_newlines=True)
+    try:
+        output = subprocess.check_output(command, universal_newlines=True)
+    except subprocess.CalledProcessError as e:
+        print(e)
+        sys.exit(1)
     print(output)
     return output.strip().splitlines()[-1]
 

src/build_context.rs

@@ -5,6 +5,7 @@ use crate::compile::warn_missing_py_init;
 use crate::module_writer::write_python_part;
 use crate::module_writer::WheelWriter;
 use crate::module_writer::{write_bin, write_bindings_module, write_cffi_module};
+use crate::source_distribution::{get_pyproject_toml, source_distribution};
 use crate::Manylinux;
 use crate::Metadata21;
 use crate::PythonInterpreter;
@@ -107,11 +108,13 @@ pub struct BuildContext {
     pub cargo_metadata: Metadata,
 }
 
+type BuiltWheelMetadata = (PathBuf, String, Option<PythonInterpreter>);
+
 impl BuildContext {
     /// Checks which kind of bindings we have (pyo3/rust-cypthon or cffi or bin) and calls the
     /// correct builder. Returns a Vec that contains location, python tag (e.g. py2.py3 or cp35)
     /// and for bindings the python interpreter they bind against.
-    pub fn build_wheels(&self) -> Result<Vec<(PathBuf, String, Option<PythonInterpreter>)>, Error> {
+    pub fn build_wheels(&self) -> Result<Vec<BuiltWheelMetadata>, Error> {
         fs::create_dir_all(&self.out)
             .context("Failed to create the target directory for the wheels")?;
 
@@ -124,6 +127,17 @@ impl BuildContext {
         Ok(wheels)
     }
 
+    /// Builds a source distribution and returns the same metadata as [BuildContext::build_wheels]
+    pub fn build_source_distribution(&self) -> Result<Option<BuiltWheelMetadata>, Error> {
+        if get_pyproject_toml(self.manifest_path.parent().unwrap()).is_ok() {
+            let sdist_path = source_distribution(&self.out, &self.metadata21, &self.manifest_path)
+                .context("Failed to build source distribution")?;
+            Ok(Some((sdist_path, "source".to_string(), None)))
+        } else {
+            Ok(None)
+        }
+    }
+
     /// Builds wheels for a Cargo project for all given python versions.
     /// Return type is the same as [BuildContext::build_wheels()]
     ///

src/develop.rs

@@ -60,7 +60,11 @@ pub fn develop(
             // Copy the artifact into the same folder as pip and python
             let bin_name = artifact.file_name().unwrap();
             let bin_path = target.get_venv_bin_dir(&venv_dir).join(bin_name);
-            fs::copy(artifact, bin_path)?;
+            fs::copy(&artifact, &bin_path).context(format!(
+                "Failed to copy {} to {}",
+                artifact.display(),
+                bin_path.display()
+            ))?;
         }
         BridgeModel::Cffi => {
             let artifact = build_context.compile_cdylib(None, None).context(context)?;

src/main.rs

@@ -125,6 +125,9 @@ enum Opt {
         /// Strip the library for minimum file size
         #[structopt(long)]
         strip: bool,
+        /// Don't build a source distribution
+        #[structopt(long = "no-sdist")]
+        no_sdist: bool,
     },
     #[cfg(feature = "upload")]
     #[structopt(name = "publish")]
@@ -134,6 +137,9 @@ enum Opt {
         build: BuildOptions,
         #[structopt(flatten)]
         publish: PublishOpt,
+        /// Don't build a source distribution
+        #[structopt(long = "no-sdist")]
+        no_sdist: bool,
     },
     #[structopt(name = "list-python")]
     /// Searches and lists the available python installations
@@ -224,18 +230,33 @@ fn run() -> Result<(), Error> {
             build,
             release,
             strip,
+            no_sdist,
         } => {
-            build.into_build_context(release, strip)?.build_wheels()?;
+            let build_context = build.into_build_context(release, strip)?;
+            if !no_sdist {
+                build_context.build_source_distribution()?;
+            }
+            build_context.build_wheels()?;
         }
         #[cfg(feature = "upload")]
-        Opt::Publish { build, publish } => {
+        Opt::Publish {
+            build,
+            publish,
+            no_sdist,
+        } => {
             let build_context = build.into_build_context(!publish.debug, !publish.no_strip)?;
 
             if !build_context.release {
                 eprintln!("⚠ Warning: You're publishing debug wheels");
             }
 
-            let wheels = build_context.build_wheels()?;
+            let mut wheels = build_context.build_wheels()?;
+
+            if !no_sdist {
+                if let Some(source_distribution) = build_context.build_source_distribution()? {
+                    wheels.push(source_distribution);
+                }
+            }
 
             let (mut registry, reenter) = complete_registry(&publish)?;
 
@@ -375,8 +396,8 @@ fn run() -> Result<(), Error> {
                 let manifest_dir = manifest_path.parent().unwrap();
                 let metadata21 = Metadata21::from_cargo_toml(&cargo_toml, &manifest_dir)
                     .context("Failed to parse Cargo.toml into python metadata")?;
-
-                let path = source_distribution(sdist_directory, &metadata21, &manifest_dir)?;
+                let path = source_distribution(sdist_directory, &metadata21, &manifest_path)
+                    .context("Failed to build source distribution")?;
                 println!("{}", path.display());
             }
         },

src/module_writer.rs

@@ -94,10 +94,11 @@ impl PathWriter {
     /// Removes a directory relative to the base path if it exists.
     ///
     /// This is to clean up the contents of an older develop call
-    pub fn delete_dir(&self, relative: impl AsRef<Path>) -> Result<(), io::Error> {
+    pub fn delete_dir(&self, relative: impl AsRef<Path>) -> Result<(), Error> {
         let absolute = self.base_path.join(relative);
         if absolute.exists() {
-            fs::remove_dir_all(absolute)?;
+            fs::remove_dir_all(&absolute)
+                .context(format!("Failed to remove {}", absolute.display()))?;
         }
 
         Ok(())

src/source_distribution.rs

@@ -1,9 +1,10 @@
 use crate::module_writer::ModuleWriter;
 use crate::{Metadata21, SDistWriter};
 use failure::{bail, Error, ResultExt};
+use serde::{Deserialize, Serialize};
 use std::path::{Path, PathBuf};
-use std::process::{Command, Stdio};
-use std::str;
+use std::process::Command;
+use std::{fs, str};
 
 /// Creates a source distribution
 ///
@@ -14,19 +15,19 @@ use std::str;
 pub fn source_distribution(
     wheel_dir: impl AsRef<Path>,
     metadata21: &Metadata21,
-    manifest_dir: impl AsRef<Path>,
+    manifest_path: impl AsRef<Path>,
 ) -> Result<PathBuf, Error> {
     let output = Command::new("cargo")
         .args(&["package", "--list", "--allow-dirty", "--manifest-path"])
-        .arg(manifest_dir.as_ref())
-        .stderr(Stdio::inherit())
+        .arg(manifest_path.as_ref())
         .output()
         .context("Failed to run cargo")?;
     if !output.status.success() {
         bail!(
-            "Failed to query file list from cargo: {}\n--- Stdout:\n{}",
+            "Failed to query file list from cargo: {}\n--- Stdout:\n{}\n--- Stderr:\n{}",
             output.status,
             String::from_utf8_lossy(&output.stdout),
+            String::from_utf8_lossy(&output.stderr),
         );
     }
 
@@ -39,8 +40,8 @@ pub fn source_distribution(
     let mut writer = SDistWriter::new(wheel_dir, &metadata21)?;
     for relative_to_cwd in file_list {
         let relative_to_project_root = relative_to_cwd
-            .strip_prefix(manifest_dir.as_ref().parent().unwrap())
-            .context("Cargo returned an out-of-tree path ಠ_ಠ")?;
+            .strip_prefix(manifest_path.as_ref().parent().unwrap())
+            .unwrap_or(relative_to_cwd);
         writer.add_file(relative_to_project_root, relative_to_cwd)?;
     }
 
@@ -55,3 +56,20 @@ pub fn source_distribution(
 
     Ok(source_distribution_path)
 }
+
+/// A pyproject.toml as specified in PEP 517
+#[derive(Serialize, Deserialize, Debug, Clone)]
+#[serde(rename_all = "kebab-case")]
+pub(crate) struct PyProjectToml {
+    build_system: toml::Value,
+}
+
+/// Returns the contents of a pyproject.toml with a `[build-system]` entry or an error
+///
+/// Does no specific error handling because it's only used to check whether or not to build
+/// source distributions
+pub(crate) fn get_pyproject_toml(project_root: impl AsRef<Path>) -> Result<PyProjectToml, Error> {
+    let contents = fs::read_to_string(project_root.as_ref().join("pyproject.toml"))?;
+    let cargo_toml = toml::from_str(&contents)?;
+    Ok(cargo_toml)
+}