fix: Fix incorrect archive construction in wasmer publish

A recent addition introduced a bug that prevented the manifest from being correctly added to the tar archive used during the publish process. Also adds a test.
wasmerio · Jun 7, 2023 · 1dfd102 · 1dfd102
1 parent 7379454
commit 1dfd102
Show file tree

Hide file tree

Showing 3 changed files with 149 additions and 48 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/lib/registry/Cargo.toml b/lib/registry/Cargo.toml
@@ -46,3 +46,6 @@ wasmparser = { version = "0.51.4", optional = true }
 rpassword = { version = "7.2.0", optional = true }
 minisign = { version = "0.7.2", optional = true }
 reqwest = { version = "0.11.12", default-features = false, features = ["blocking", "multipart", "json", "stream"] }
+
+[dev-dependencies]
+pretty_assertions = "1.3.0"
diff --git a/lib/registry/src/package/builder.rs b/lib/registry/src/package/builder.rs
@@ -59,8 +59,6 @@ enum PackageBuildError {
 impl Publish {
     /// Executes `wasmer publish`
     pub fn execute(&self) -> Result<(), anyhow::Error> {
-        let mut builder = Builder::new(Vec::new());
-
         let input_path = match self.package_path.as_ref() {
             Some(s) => std::env::current_dir()?.join(s),
             None => std::env::current_dir()?,
@@ -118,6 +116,9 @@ impl Publish {
             manifest.package.version = version.clone();
         }
 
+        let archive_dir = tempfile::TempDir::new()?;
+        let archive_meta = construct_tar_gz(&archive_dir.path(), &manifest, &manifest_path)?;
+
         let registry = match self.registry.as_deref() {
             Some(s) => crate::format_graphql(s),
             None => {
@@ -130,30 +131,11 @@ impl Publish {
         };
 
         if !self.no_validate {
-            validate::validate_directory(&manifest, &registry, manifest_dir.clone())?;
+            validate::validate_directory(&manifest, &registry, manifest_dir)?;
         }
 
-        builder.append_path_with_name(&manifest_dir, PACKAGE_TOML_FALLBACK_NAME)?;
-
-        let manifest_string = toml::to_string(&manifest)?;
-
-        let (readme, license) = construct_tar_gz(&mut builder, &manifest, &manifest_dir)?;
-
-        builder
-            .finish()
-            .map_err(|e| anyhow::anyhow!("failed to finish .tar.gz builder: {e}"))?;
-        let tar_archive_data = builder.into_inner().expect("tar archive was not finalized");
-        let archive_name = "package.tar.gz".to_string();
-        let archive_dir = tempfile::TempDir::new()?;
-        let archive_dir_path: &std::path::Path = archive_dir.as_ref();
-        fs::create_dir(archive_dir_path.join("wapm_package"))?;
-        let archive_path = archive_dir_path.join("wapm_package").join(&archive_name);
-        let mut compressed_archive = fs::File::create(&archive_path).unwrap();
-        let mut gz_enc = GzEncoder::new(&mut compressed_archive, Compression::best());
-
-        gz_enc.write_all(&tar_archive_data).unwrap();
-        let _compressed_archive = gz_enc.finish().unwrap();
-        let mut compressed_archive_reader = fs::File::open(&archive_path)?;
+        let archive_path = &archive_meta.archive_path;
+        let mut compressed_archive_reader = fs::File::open(archive_path)?;
 
         let maybe_signature_data = sign_compressed_archive(&mut compressed_archive_reader)?;
         let archived_data_size = archive_path.metadata()?.len();
@@ -169,6 +151,9 @@ impl Publish {
                 manifest.package.name, manifest.package.version
             );
 
+            let path = archive_dir.into_path();
+            eprintln!("Archive persisted at: {}", path.display());
+
             log::info!(
                 "Publish succeeded, but package was not published because it was run in dry-run mode"
             );
@@ -180,57 +165,83 @@ impl Publish {
             Some(registry),
             self.token.clone(),
             &manifest.package,
-            &manifest_string,
-            &license,
-            &readme,
-            &archive_name,
-            &archive_path,
+            &archive_meta.manifest_toml,
+            &archive_meta.license,
+            &archive_meta.readme,
+            &archive_meta
+                .archive_path
+                .file_name()
+                .unwrap()
+                .to_str()
+                .unwrap()
+                .to_string(),
+            archive_path,
             &maybe_signature_data,
             archived_data_size,
             self.quiet,
         )
     }
 }
 
+struct ConstructedPackageArchive {
+    manifest_toml: String,
+    readme: Option<String>,
+    license: Option<String>,
+    archive_path: PathBuf,
+}
+
 fn construct_tar_gz(
-    builder: &mut tar::Builder<Vec<u8>>,
+    archive_dir: &Path,
     manifest: &wasmer_toml::Manifest,
-    cwd: &Path,
-) -> Result<(Option<String>, Option<String>), anyhow::Error> {
+    manifest_path: &Path,
+) -> Result<ConstructedPackageArchive, anyhow::Error> {
+    // This is an assert instead of returned error because this is a programmer error.
+    debug_assert!(manifest_path.is_file(), "manifest path is not a file");
+
+    let manifest_dir = manifest_path
+        .parent()
+        .context("manifest path has no parent directory")?;
+
+    let mut builder = Builder::new(Vec::new());
+    builder.append_path_with_name(manifest_path, PACKAGE_TOML_FALLBACK_NAME)?;
+
+    let manifest_string = toml::to_string(&manifest)?;
+
     let package = &manifest.package;
     let modules = manifest.module.as_deref().unwrap_or_default();
 
     let readme = match package.readme.as_ref() {
         None => None,
         Some(s) => {
-            let path = append_path_to_tar_gz(builder, &manifest.base_directory_path, s).map_err(
-                |(p, e)| PackageBuildError::ErrorBuildingPackage(format!("{}", p.display()), e),
-            )?;
-            fs::read_to_string(path).ok()
+            let path = append_path_to_tar_gz(&mut builder, &manifest.base_directory_path, s)
+                .map_err(|(p, e)| {
+                    PackageBuildError::ErrorBuildingPackage(format!("{}", p.display()), e)
+                })?;
+            Some(std::fs::read_to_string(path)?)
         }
     };
 
-    let license_file = match package.license_file.as_ref() {
+    let license = match package.license_file.as_ref() {
         None => None,
         Some(s) => {
-            let path = append_path_to_tar_gz(builder, &manifest.base_directory_path, s).map_err(
-                |(p, e)| PackageBuildError::ErrorBuildingPackage(format!("{}", p.display()), e),
-            )?;
-            fs::read_to_string(path).ok()
+            let path = append_path_to_tar_gz(&mut builder, &manifest.base_directory_path, s)
+                .map_err(|(p, e)| {
+                    PackageBuildError::ErrorBuildingPackage(format!("{}", p.display()), e)
+                })?;
+            Some(std::fs::read_to_string(path)?)
         }
     };
 
     for module in modules {
-        append_path_to_tar_gz(builder, &manifest.base_directory_path, &module.source).map_err(
-            |(normalized_path, _)| PackageBuildError::SourceMustBeFile {
+        append_path_to_tar_gz(&mut builder, &manifest.base_directory_path, &module.source)
+            .map_err(|(normalized_path, _)| PackageBuildError::SourceMustBeFile {
                 module: module.name.clone(),
                 path: normalized_path,
-            },
-        )?;
+            })?;
 
         if let Some(bindings) = &module.bindings {
             for path in bindings.referenced_files(&manifest.base_directory_path)? {
-                append_path_to_tar_gz(builder, &manifest.base_directory_path, &path).map_err(
+                append_path_to_tar_gz(&mut builder, &manifest.base_directory_path, &path).map_err(
                     |(normalized_path, _)| PackageBuildError::MissingBindings {
                         module: module.name.clone(),
                         path: normalized_path,
@@ -243,7 +254,7 @@ fn construct_tar_gz(
     // bundle the package filesystem
     let default = indexmap::IndexMap::default();
     for (_alias, path) in manifest.fs.as_ref().unwrap_or(&default).iter() {
-        let normalized_path = normalize_path(cwd, path);
+        let normalized_path = normalize_path(manifest_dir, path);
         let path_metadata = normalized_path.metadata().map_err(|_| {
             PackageBuildError::MissingManifestFsPath(normalized_path.to_string_lossy().to_string())
         })?;
@@ -260,7 +271,25 @@ fn construct_tar_gz(
         })?;
     }
 
-    Ok((readme, license_file))
+    builder
+        .finish()
+        .map_err(|e| anyhow::anyhow!("failed to finish .tar.gz builder: {e}"))?;
+    let tar_archive_data = builder.into_inner().expect("tar archive was not finalized");
+    let archive_name = "package.tar.gz".to_string();
+    fs::create_dir(archive_dir.join("wapm_package"))?;
+    let archive_path = archive_dir.join("wapm_package").join(archive_name);
+    let mut compressed_archive = fs::File::create(&archive_path).unwrap();
+    let mut gz_enc = GzEncoder::new(&mut compressed_archive, Compression::best());
+
+    gz_enc.write_all(&tar_archive_data).unwrap();
+    let _compressed_archive = gz_enc.finish().unwrap();
+
+    Ok(ConstructedPackageArchive {
+        manifest_toml: manifest_string,
+        archive_path,
+        readme,
+        license,
+    })
 }
 
 fn append_path_to_tar_gz(
@@ -689,3 +718,71 @@ mod validate {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::io::Read;
+
+    use super::*;
+
+    #[test]
+    fn test_construct_package_tar_gz() {
+        let manifest_str = r#"[package]
+name = "wasmer-tests/wcgi-always-panic"
+version = "0.6.0"
+description = "wasmer-tests/wcgi-always-panic website"
+
+[[module]]
+name = "wcgi-always-panic"
+source = "module.wasm"
+abi = "wasi"
+
+[[command]]
+name = "wcgi"
+module = "wcgi-always-panic"
+runner = "https://webc.org/runner/wcgi"
+"#;
+
+        let archive_dir = tempfile::tempdir().unwrap();
+
+        let manifest_dir = tempfile::tempdir().unwrap();
+
+        let mp = manifest_dir.path();
+        let manifest_path = mp.join("wasmer.toml");
+
+        std::fs::write(&manifest_path, manifest_str).unwrap();
+        std::fs::write(mp.join("module.wasm"), "()").unwrap();
+
+        let mut manifest = wasmer_toml::Manifest::parse(&manifest_str).unwrap();
+        manifest.base_directory_path = manifest_dir.path().to_owned();
+
+        let meta = construct_tar_gz(&archive_dir.path(), &manifest, &manifest_path).unwrap();
+
+        let mut data = std::io::Cursor::new(std::fs::read(&meta.archive_path).unwrap());
+
+        let gz = flate2::read::GzDecoder::new(&mut data);
+        let mut archive = tar::Archive::new(gz);
+
+        let map = archive
+            .entries()
+            .unwrap()
+            .map(|res| {
+                let mut entry = res.unwrap();
+                let name = entry.path().unwrap().to_str().unwrap().to_string();
+                let mut contents = String::new();
+                entry.read_to_string(&mut contents).unwrap();
+                eprintln!("{name}:\n{contents}\n\n");
+                (name, contents)
+            })
+            .collect::<std::collections::HashMap<String, String>>();
+
+        let expected: std::collections::HashMap<String, String> = [
+            ("wapm.toml".to_string(), manifest_str.to_string()),
+            ("module.wasm".to_string(), "()".to_string()),
+        ]
+        .into_iter()
+        .collect();
+
+        pretty_assertions::assert_eq!(map, expected);
+    }
+}