20240523 ensure to regenerate lockfiles if vendor set to false (#84)

Might close #82 not sure if that fixes that

---------

Signed-off-by: Soc Virnyl Estela <[email protected]>

Cargo.toml

@@ -12,7 +12,7 @@ default-members = [
 resolver = "2"
 
 [workspace.package]
-version = "1.3.2"
+version = "1.3.5-dev"
 description = "OBS Source Service and utilities for Rust software packaging."
 authors = [
   "Soc Virnyl Estela <[email protected]>",

README.md

@@ -143,6 +143,14 @@ paths to be part of the vendored tarball. So a path that looks like
 if extracted, it will go to the desired path `rust/pv/Cargo.lock` from
 the root folder of the project.
 
+## Respecting lockfiles
+
+A new option is added to respect lockfiles. This means that vendored tarballs
+are expected to have the same metadata inside the `Cargo.lock`.
+
+> ![WARNING] `cargo-vendor-filterer` is not supported for lockfile validation/verification
+at this time.
+
 # How to do multiple vendors
 
 It is possible to do multiple vendored tarballs by using the `--tag` parameter. This allows you to rename your vendored

bulk-updater/Cargo.toml

@@ -10,7 +10,7 @@ license.workspace = true
 
 [dependencies]
 quick-xml = { workspace = true, features = ["overlapped-lists", "serialize"] }
-obs-service-cargo = { path = "../cargo", version = "1" }
+obs-service-cargo = { path = "../cargo" }
 clap = { workspace = true, features = ["derive"] }
 serde = { workspace = true, features = ["derive", "alloc"] }
 tracing.workspace = true

bulk-updater/src/operations.rs

@@ -288,6 +288,7 @@ pub fn attempt_cargo_update_before_revendor(
         let mut src: String = String::new();
         let mut tag: Option<String> = None;
         let mut filter = false; // Still experimental, so default=false for now
+        let mut respect_lockfile = true;
         let outdir = package_path.to_path_buf();
         for param in params.iter() {
             if let Some(name) = param.name.clone() {
@@ -323,6 +324,11 @@ pub fn attempt_cargo_update_before_revendor(
                         filter = val;
                     }
                 };
+                if name == "respect_lockfile" {
+                    if let Some(val) = param.text.as_ref().and_then(|t| t.parse::<bool>().ok()) {
+                        respect_lockfile = val;
+                    }
+                };
             }
         }
         if compression.is_empty() || src.is_empty() {
@@ -353,6 +359,7 @@ pub fn attempt_cargo_update_before_revendor(
             color: colorize,
             i_accept_the_risk: accept_risks,
             filter,
+            respect_lockfile,
         };
         srcpath
             .run_vendor(&new_opts)

cargo/src/cli.rs

@@ -71,6 +71,8 @@ pub struct Opts {
         help = "A list of rustsec-id's to ignore. By setting this value, you acknowledge that this issue does not affect your package and you should be exempt from resolving it."
     )]
     pub i_accept_the_risk: Vec<String>,
+    #[arg(long, default_value_t = true, action = clap::ArgAction::Set, help = "Respect lockfile or not if it exists. Otherwise, regenerate the lockfile and try to respect the lockfile.")]
+    pub respect_lockfile: bool,
 }
 
 impl AsRef<Opts> for Opts {

cargo/src/errors.rs

@@ -7,6 +7,7 @@ pub enum OBSCargoErrorKind {
     VendorCompressionFailed,
     VendorError,
     AuditError,
+    LockFileError,
 }
 
 impl OBSCargoErrorKind {
@@ -17,6 +18,7 @@ impl OBSCargoErrorKind {
             AuditNeedsAction => "security audit is actionable",
             VendorError => "cargo vendor process failed",
             VendorCompressionFailed => "compress vendored dependencies failed",
+            LockFileError => "lockfile generation failed",
         }
     }
 }

cargo/src/utils/compress.rs

@@ -57,7 +57,7 @@ fn add_path_to_archive<T: Write>(
     } else {
         error!("Ignoring unexpected special file: {:?}", path);
     }
-    debug!("Added {} to archive", path.to_string_lossy());
+    trace!("Added {} to archive", path.to_string_lossy());
     Ok(())
 }
 

cargo/src/utils/mod.rs

@@ -20,7 +20,7 @@ use std::path::PathBuf;
 use crate::cli::{Compression, Opts};
 use crate::errors::OBSCargoError;
 use crate::errors::OBSCargoErrorKind;
-use crate::vendor::{self, vendor};
+use crate::vendor::{self, generate_lockfile, vendor};
 
 use crate::audit::{perform_cargo_audit, process_reports};
 
@@ -86,7 +86,7 @@ pub fn process_src(args: &Opts, prjdir: &Path) -> Result<(), OBSCargoError> {
 
     // Setup some common paths we'll use from here out.
     let outdir = args.outdir.to_owned();
-    let cargo_config = prjdir.join(".cargo/config");
+    let cargo_config = prjdir.join(".cargo/config.toml");
     let vendor_dir = prjdir.join("vendor");
     let update = args.update;
 
@@ -187,6 +187,8 @@ pub fn process_src(args: &Opts, prjdir: &Path) -> Result<(), OBSCargoError> {
     // if there is `None`.
     let mut cargo_locks: Vec<PathBuf> = Vec::new();
 
+    // Let's ensure the lockfiles are generated even if they don't exist
+    // This guarantees that the dependencies used are properly recorded
     for manifest_file in manifest_files.iter() {
         let manifest_f = manifest_file.canonicalize().map_err(|err| {
             error!("Failed to canonicalize path: {}", err);
@@ -196,7 +198,19 @@ pub fn process_src(args: &Opts, prjdir: &Path) -> Result<(), OBSCargoError> {
         let lockfile_path = manifest_f.parent().map(|path_f| path_f.join("Cargo.lock"));
         if let Some(lockfile_p) = lockfile_path {
             if lockfile_p.exists() {
+                debug!("Path to extra lockfile: {}", lockfile_p.display());
                 cargo_locks.push(lockfile_p)
+            } else {
+                debug!("Path to extra lockfile not found: {}", lockfile_p.display());
+                if generate_lockfile(manifest_file).is_ok() {
+                    info!(
+                        "🔒 Cargo lockfile created for extra lockfile at path: {}",
+                        lockfile_p.display()
+                    );
+                    cargo_locks.push(lockfile_p)
+                } else {
+                    debug!("Path didn't generate manifest: {}", lockfile_p.display());
+                }
             };
         };
     }
@@ -208,6 +222,20 @@ pub fn process_src(args: &Opts, prjdir: &Path) -> Result<(), OBSCargoError> {
         if lockfilepath.exists() {
             debug!("Path to first cargo lock: {}", lockfilepath.display());
             cargo_locks.push(lockfilepath);
+        } else {
+            debug!(
+                "Path to first cargo lock not found: {}",
+                lockfilepath.display()
+            );
+            if generate_lockfile(&first_manifest).is_ok() {
+                info!(
+                    "🔒 Cargo lockfile created for first lockfile at path: {}",
+                    lockfilepath.display()
+                );
+                cargo_locks.push(lockfilepath);
+            } else {
+                debug!("Path didn't generate manifest: {}", lockfilepath.display());
+            };
         };
     };
 
@@ -234,6 +262,7 @@ pub fn process_src(args: &Opts, prjdir: &Path) -> Result<(), OBSCargoError> {
             &first_manifest,
             &manifest_files,
             args.filter,
+            args.respect_lockfile,
         )?;
 
         // Finally, compress everything together.
@@ -329,7 +358,6 @@ pub fn cargo_command<S: AsRef<OsStr>>(
     subcommand: &str,
     options: &[S],
     curdir: impl AsRef<Path>,
-    // TODO ExecutionError should also have error output as String :)
 ) -> Result<String, ExecutionError> {
     let cmd = std::process::Command::new("cargo")
         .arg(subcommand)

cargo/src/vendor.rs

@@ -47,12 +47,45 @@ pub fn update(
     })
 }
 
+pub fn generate_lockfile(manifest_path: impl AsRef<Path>) -> Result<(), OBSCargoError> {
+    let lockfile_options: Vec<OsString> = vec![
+        "-vv".into(),
+        "--manifest-path".into(),
+        manifest_path.as_ref().into(),
+    ];
+
+    let parent_path = if let Some(the_parent) = manifest_path.as_ref().parent() {
+        the_parent.to_path_buf()
+    } else {
+        let guess_path = std::env::current_dir().map_err(|e| {
+            error!(err = %e);
+            OBSCargoError::new(
+                OBSCargoErrorKind::LockFileError,
+                "Getting parent path for lockfile generation failed".into(),
+            )
+        })?;
+        guess_path.to_path_buf()
+    };
+
+    Ok({
+        cargo_command("generate-lockfile", &lockfile_options, parent_path).map_err(|e| {
+            error!(err = %e);
+            OBSCargoError::new(
+                OBSCargoErrorKind::LockFileError,
+                "Unable to generate a lockfile".into(),
+            )
+        })?;
+        info!("🔒 Successfully generated lockfile")
+    })
+}
+
 pub fn vendor(
     prjdir: impl AsRef<Path>,
     cargo_config: impl AsRef<Path>,
     manifest_path: impl AsRef<Path>,
     extra_manifest_paths: &[impl AsRef<Path>],
     filter: bool,
+    respect_lockfile: bool,
 ) -> Result<(), OBSCargoError> {
     let mut vendor_options: Vec<OsString> =
         vec!["--manifest-path".into(), manifest_path.as_ref().into()];
@@ -78,10 +111,18 @@ pub fn vendor(
         // with using `--format=tar.zstd` for example. But we need to include
         // additional files and it also doesn't support all compression-schemes.
         vendor_options.push("--format=dir".into());
+        if respect_lockfile {
+            info!("⚠️ Using vendor-filterer, lockfile verification not supported");
+        };
         "vendor-filterer"
     } else {
         // cargo-vendor-filterer doesn't support `-vv`
         vendor_options.push("-vv".into());
+        // Enforce lock is up-to-date despite the fact we are regenerating the locks
+        if respect_lockfile {
+            // NOTE: Only vendor has the --locked option
+            vendor_options.push("--locked".into());
+        };
         "vendor"
     };
 

cargo_vendor.service

@@ -36,4 +36,9 @@
     <allowedvalue>false</allowedvalue>
     <allowedvalue>true</allowedvalue>
   </parameter>
+  <parameter name="respect-lockfile">
+    <description>Attempt to respect lockfile if it exists. Otherwise, attempt to regenerate lockfile and attempt to respect the new lockfile.</description>
+    <allowedvalue>false</allowedvalue>
+    <allowedvalue>true</allowedvalue>
+  </parameter>
 </service>

runtests.sh

@@ -3,6 +3,7 @@
 set -euo pipefail
 
 SCRIPTPATH="$( cd "$(dirname "$0")" || exit ; pwd -P )"
+export RUST_LOG=debug
 
 echo "# Downloading bonk"
 curl -LJ0 "https://github.com/elliot40404/bonk/archive/refs/tags/v0.3.2.tar.gz" --output /tmp/bonk-0.3.2.tar.gz
@@ -23,6 +24,13 @@ echo "# No tarball to remove"
 
 echo "# Generating tarball"
 "${SCRIPTPATH}"/target/release/cargo_vendor --src /tmp/s390-tools-2.29.0.tar.gz --outdir /tmp --cargotoml rust/pvsecret/Cargo.toml --cargotoml rust/utils/Cargo.toml --cargotoml rust/pv/Cargo.toml
+echo "# Multiple vendors with tags"
+"${SCRIPTPATH}"/target/release/cargo_vendor --src /tmp/s390-tools-2.29.0.tar.gz --outdir /tmp --cargotoml rust/pvsecret/Cargo.toml --tag pvsecret
+"${SCRIPTPATH}"/target/release/cargo_vendor --src /tmp/s390-tools-2.29.0.tar.gz --outdir /tmp --cargotoml rust/pv/Cargo.toml --tag rust-pv
+"${SCRIPTPATH}"/target/release/cargo_vendor --src /tmp/s390-tools-2.29.0.tar.gz --outdir /tmp --cargotoml rust/utils/Cargo.toml --tag utils
+echo "# Test only one tag with multiple cargotoml"
+"${SCRIPTPATH}"/target/release/cargo_vendor --src /tmp/s390-tools-2.29.0.tar.gz --outdir /tmp --cargotoml rust/pvsecret/Cargo.toml --cargotoml rust/utils/Cargo.toml --cargotoml rust/pv/Cargo.toml --tag "rust-component"
+
 echo "# Removing vendored tarball"
 rm /tmp/vendor.tar.zst
 
@@ -41,9 +49,6 @@ echo "# Generating tarball"
 echo "# Removing vendored tarball"
 rm /tmp/vendor.tar.zst
 
-echo "# Test tagging"
-"${SCRIPTPATH}"/target/release/cargo_vendor --src /tmp/s390-tools-2.29.0.tar.gz --outdir /tmp --cargotoml rust/pvsecret/Cargo.toml --cargotoml rust/utils/Cargo.toml --cargotoml rust/pv/Cargo.toml --tag "rust-component"
-
 if [ ! -f "/tmp/vendor-rust-component.tar.zst" ]
 then
     # Fail