chore: adds tests, clippy, formatting, and auditing to CI #56

dyro · 2024-10-28T21:12:55Z

No description provided.

dyro · 2024-10-28T22:20:52Z

Cargo.toml

@@ -20,7 +22,6 @@ thiserror = "1.0.49"
 uniffi = "0.24.1"
 openssl-src = "=300.3.1" # Required for openssl-sys
 log = "0.4.21"
-env_logger = "0.11.3"


Unused dependency.

dyro · 2024-10-28T22:21:10Z

Cargo.toml

@@ -9,6 +9,8 @@ authors = ["Gavin Peacock <[email protected]"]
 name = "c2pa"
 crate-type = ["cdylib"]

+[package.metadata.cargo-udeps.ignore]
+normal = ["openssl-src"]


We use this to explicitly set the openssl-src dependency.

dyro · 2024-10-28T22:22:54Z

src/json_api.rs

-
-/// Returns ManifestStore JSON string from a file path.
-///
-/// If data_dir is provided, any thumbnail or c2pa data will be written to that folder.
-/// Any Validation errors will be reported in the validation_status field.
-///
-pub fn read_file(path: &str, data_dir: Option<String>) -> Result<String> {
-    Ok(match data_dir {
-        Some(dir) => ManifestStore::from_file_with_resources(path, &dir),
-        None => ManifestStore::from_file(path),
-    }
-    .map_err(Error::from_c2pa_error)?
-    .to_string())
-}
-
-/// Returns an Ingredient JSON string from a file path.
-///
-/// Any thumbnail or c2pa data will be written to data_dir if provided
-pub fn read_ingredient_file(path: &str, data_dir: &str) -> Result<String> {
-    Ok(Ingredient::from_file_with_folder(path, data_dir)
-        .map_err(Error::from_c2pa_error)?
-        .to_string())
-}
-
-/// Adds a manifest to the source file and writes the result to the destination file.
-/// Also returns the binary manifest data for optional cloud storage
-/// A manifest definition must be supplied
-/// Signer information must also be supplied
-///
-/// Any file paths in the manifest will be read relative to the source file
-pub fn sign_file(
-    source: &str,
-    dest: &str,
-    manifest_json: &str,
-    signer_info: &SignerInfo,
-    data_dir: Option<String>,
-) -> Result<Vec<u8>> {
-    let mut manifest = Manifest::from_json(manifest_json).map_err(Error::from_c2pa_error)?;
-
-    // if data_dir is provided, set the base path for the manifest
-    if let Some(path) = data_dir {
-        manifest
-            .with_base_path(path)
-            .map_err(Error::from_c2pa_error)?;
-    }
-
-    // If the source file has a manifest store, and no parent is specified, treat the source's manifest store as the parent.
-    if manifest.parent().is_none() {
-        let source_ingredient = Ingredient::from_file(source).map_err(Error::from_c2pa_error)?;
-        if source_ingredient.manifest_data().is_some() {
-            manifest
-                .set_parent(source_ingredient)
-                .map_err(Error::from_c2pa_error)?;
-        }
-    }
-
-    let signer = signer_info.signer()?;
-    manifest
-        .embed(&source, &dest, &*signer)
-        .map_err(Error::from_c2pa_error)
-}
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-    use std::{fs::remove_dir_all, path::PathBuf};
-
-    /// returns a path to a file in the fixtures folder
-    pub fn test_path(path: &str) -> String {
-        let base = env!("CARGO_MANIFEST_DIR");
-        format!("{}/{}", base, path)
-    }
-
-    #[test]
-    fn test_verify_from_file_no_base() {
-        let path = test_path("tests/fixtures/C.jpg");
-        let result = read_file(&path, None);
-        assert!(result.is_ok());
-        let json_report = result.unwrap();
-        println!("{}", json_report);
-        assert!(json_report.contains("C.jpg"));
-        assert!(!json_report.contains("validation_status"));
-    }
-
-    #[test]
-    fn test_read_from_file_with_base() {
-        let path = test_path("tests/fixtures/C.jpg");
-        let data_dir = "target/data_dir";
-        if PathBuf::from(data_dir).exists() {
-            remove_dir_all(data_dir).unwrap();
-        }
-        let result = read_file(&path, Some(data_dir.to_owned()));
-        assert!(result.is_ok());
-        let json_report = result.unwrap();
-        println!("{}", json_report);
-        assert!(json_report.contains("C.jpg"));
-        assert!(PathBuf::from(data_dir).exists());
-        assert!(json_report.contains("thumbnail"));
-    }
-}


Removes this because it wasn't being used in the new APIs, and we've removed the v1 and add_thumbnails features this was reliant upon for being included.

dyro · 2024-10-28T22:23:14Z

src/lib.rs

-#[cfg(feature = "v1")]
-mod json_api;
-#[cfg(feature = "v1")]
-pub use json_api::{read_file, read_ingredient_file, sign_file};
-#[cfg(feature = "v1")]
-mod signer_info;
-#[cfg(feature = "v1")]
-pub use signer_info::{CallbackSigner, SignerCallback, SignerConfig, SignerInfo};


This features had already been removed from the crate. Removing the code so clippy is happy :)

Yes, it should be fine to remove this now. No one seems overly bothered by it going away.

dyro · 2024-10-28T22:23:21Z

src/lib.rs

@@ -57,6 +49,12 @@ pub struct Reader {
    reader: RwLock<c2pa::Reader>,
 }

+impl Default for Reader {


Clippy suggestion

dyro · 2024-10-28T22:24:20Z

src/signer_info.rs

-// Copyright 2023 Adobe. All rights reserved.
-// This file is licensed to you under the Apache License,
-// Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
-// or the MIT license (http://opensource.org/licenses/MIT),
-// at your option.
-// Unless required by applicable law or agreed to in writing,
-// this software is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR REPRESENTATIONS OF ANY KIND, either express or
-// implied. See the LICENSE-MIT and LICENSE-APACHE files for the
-// specific language governing permissions and limitations under
-// each license.
-
-use c2pa::{create_signer, Signer, SigningAlg};
-use serde::Deserialize;
-
-use crate::{Error, Result};
-
-/// SignerInfo provides the information needed to create a signer
-/// and sign a manifest.
-///
-/// The signer is created from the signcert and pkey fields.
-///
-/// The alg field is used to determine the signing algorithm.
-///
-/// The tsa_url field is optional and is used to specify a timestamp server.
-///
-#[derive(Clone, Debug, Default, Deserialize)]
-pub struct SignerInfo {
-    pub alg: String,
-    pub sign_cert: Vec<u8>,
-    pub private_key: Vec<u8>,
-    pub ta_url: Option<String>,
-}
-
-impl SignerInfo {
-    /// Create a SignerInfo from a JSON formatted SignerInfo string
-    pub fn from_json(json: &str) -> Result<Self> {
-        serde_json::from_str(json).map_err(|e| Error::Json {
-            reason: e.to_string(),
-        })
-    }
-
-    // Returns the signing algorithm converted from string format
-    fn alg(&self) -> Result<SigningAlg> {
-        self.alg.to_lowercase().parse().map_err(|_| Error::Other {
-            reason: "Invalid signing algorithm".to_string(),
-        })
-    }
-
-    /// Create a signer from the SignerInfo
-    pub fn signer(&self) -> Result<Box<dyn Signer>> {
-        create_signer::from_keys(
-            &self.sign_cert,
-            &self.private_key,
-            self.alg()?,
-            self.ta_url.clone(),
-        )
-        .map_err(Error::from_c2pa_error)
-    }
-}


Removing for the same reason as removing the json_api. This wasn't being used and it was hidden behind a feature flag that was removed. Lemme know if I should add this back, @gpeacock

dyro · 2024-10-28T22:25:39Z

src/streams.rs

@@ -70,7 +70,7 @@ impl<'a> StreamAdapter<'a> {
 impl<'a> From<&'a dyn Stream> for StreamAdapter<'a> {
    #[allow(invalid_reference_casting)]
    fn from(stream: &'a dyn Stream) -> Self {
-        let stream = &*stream as *const dyn Stream as *mut dyn Stream;
+        let stream = stream as *const dyn Stream as *mut dyn Stream;


The old version and new version are the same type, as pointed out by clippy. Lemme know if I'm missing something, @gpeacock. I know the binding stuff can be crazy. Maybe the &* is required anyway?

I'll trust clippy on this one. That's a remnant of older code.

dyro · 2024-10-28T22:32:01Z

src/error.rs

-            #[cfg(feature = "add_thumbnails")]
-            ImageError => Self::ImageError(err_str),


Same here: this feature was removed when we swapped to the V24 API 👍

gpeacock · 2024-10-29T00:51:54Z

src/lib.rs

-#[cfg(feature = "v1")]
-mod json_api;
-#[cfg(feature = "v1")]
-pub use json_api::{read_file, read_ingredient_file, sign_file};
-#[cfg(feature = "v1")]
-mod signer_info;
-#[cfg(feature = "v1")]
-pub use signer_info::{CallbackSigner, SignerCallback, SignerConfig, SignerInfo};


Yes, it should be fine to remove this now. No one seems overly bothered by it going away.

changes the following to appease the new CI rules: * ignores unused dep warning for openssl-sys. * runs cargo +nightly fmt on source. * fixes clippy issues. * removes modules from V1 that weren't being used. These modules were behind v1 feature flags that had been removed as well. * adds deny.toml from c2pa-rs.

dyro added the ready to test label Oct 28, 2024

dyro force-pushed the dyross/CI branch 3 times, most recently from 4a8351a to 4315e49 Compare October 28, 2024 22:20

dyro commented Oct 28, 2024

View reviewed changes

dyro requested review from gpeacock and tmathern October 28, 2024 22:26

dyro commented Oct 28, 2024

View reviewed changes

dyro force-pushed the dyross/CI branch from 4315e49 to 308c560 Compare October 28, 2024 22:50

tmathern approved these changes Oct 28, 2024

View reviewed changes

gpeacock approved these changes Oct 29, 2024

View reviewed changes

dyro force-pushed the dyross/CI branch 3 times, most recently from f9720b1 to ab84539 Compare October 29, 2024 03:15

dyro removed the ready to test label Oct 30, 2024

dyro force-pushed the dyross/CI branch from ab84539 to 1b00eb1 Compare October 30, 2024 16:57

dyro added safe to test and removed safe to test labels Oct 30, 2024

dyro force-pushed the dyross/CI branch from 1b00eb1 to 345aba5 Compare October 30, 2024 17:05

dyro merged commit dff93a0 into main Oct 30, 2024
23 checks passed

dyro deleted the dyross/CI branch October 30, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: adds tests, clippy, formatting, and auditing to CI #56

chore: adds tests, clippy, formatting, and auditing to CI #56

dyro commented Oct 28, 2024

dyro Oct 28, 2024

dyro Oct 28, 2024

dyro Oct 28, 2024

dyro Oct 28, 2024

gpeacock Oct 29, 2024

dyro Oct 28, 2024

dyro Oct 28, 2024

dyro Oct 28, 2024

gpeacock Oct 29, 2024

dyro Oct 28, 2024

gpeacock Oct 29, 2024

		#[cfg(feature = "add_thumbnails")]
		ImageError => Self::ImageError(err_str),

chore: adds tests, clippy, formatting, and auditing to CI #56

chore: adds tests, clippy, formatting, and auditing to CI #56

Conversation

dyro commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment