Repository: shssoichiro/oxipng
Pull request #491 (2023-04-07 05:48:29)

Update CI
* add `workflow_dispatch` for manual running
* add `fail-fast` false in matrix
* specify `persist-credentials: false` for actions/checkout
* update actions to the latest versions
* move cache before installing the toolchain
* cache more stuff
* limit deploy to shssoichiro/oxipng repository
* reindent

Non-whitespace diff: https://github.com/shssoichiro/oxipng/pull/491/files?w=1

---

There's one warning which needs to be fixed later regarding `set-output`:

>The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

The other warning about Node.js 12 actions that were not updated in this PR because there's no newer version, needs to be fixed upstream, e.g. in https://github.com/actions-rs/toolchain:

> Node.js 12 actions are deprecated. Please update the following actions to use Node.js 16: actions/checkout@v2, actions-rs/toolchain@v1. For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/.


XhmikosR: There's one warning which needs to be fixed later regarding `set-output`:

>The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/
AlexTMjugador: > The other warning about Node.js 12 actions that were not updated in this PR because there's no newer version, needs to be fixed upstream, e.g. in https://github.com/actions-rs/toolchain

The repositories at the `actions-rs` organization are abandoned, so it's unlikely that they will be updated soon. I'd suggest migrating to alternative actions, such as [`dtolnay/rust-toolchain`](https://github.com/dtolnay/rust-toolchain).
XhmikosR: Yeah, someone else can take care of that later.
XhmikosR: All right, I made a couple more tweaks. Notes:

1. clippy seems to fail from forks, but the errors aren't fatal. Ideally it should be skipped when not running from the main repo, see https://github.com/actions-rs/clippy-check/issues/2. Another solution would be to move clippy to its own workflow and just run it on pushes only
2. the https://github.com/actions-rs/toolchain action needs to be replaced by someone more familiar with rust later with https://github.com/dtolnay/rust-toolchain
3. cache could be used in the deploy workflow too later
4. bonus: we could upload the binaries for each run so that people can easily try "nightlies"

Patch:
diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml
index 01ae14f8..ce1a9032 100644
--- a/.github/workflows/deploy.yml
+++ b/.github/workflows/deploy.yml
@@ -6,8 +6,9 @@ on:
       - 'v*.*.*'
 
 jobs:
-
   create-windows-binaries:
+    runs-on: windows-latest
+    if: github.repository == 'shssoichiro/oxipng'
 
     strategy:
       matrix:
@@ -16,51 +17,53 @@ jobs:
 # and I don't have a Windows machine set up to experiment with fixing it.
 #        conf: [x86_64, i686]
 
-    runs-on: windows-latest
-
     steps:
-    - uses: actions/checkout@v2
-
-    - name: Install stable
-      uses: actions-rs/toolchain@v1
-      with:
-        profile: minimal
-        toolchain: stable
-        target: ${{ matrix.conf }}-pc-windows-msvc
-        override: true
-
-    - name: Build oxipng
-      run: |
-        cargo build --release --target ${{ matrix.conf }}-pc-windows-msvc
-
-    - name: Get the version
-      shell: bash
-      id: tagName
-      run: |
-        VERSION=$(cargo pkgid | cut -d# -f2 | cut -d: -f2)
-        echo "::set-output name=tag::$VERSION"
-
-    - name: Build package
-      id: package
-      shell: bash
-      run: |
-        ARCHIVE_TARGET="${{ matrix.conf }}-pc-windows-msvc"
-        ARCHIVE_NAME="oxipng-${{ steps.tagName.outputs.tag }}-$ARCHIVE_TARGET"
-        ARCHIVE_FILE="${ARCHIVE_NAME}.zip"
-        mv LICENSE LICENSE.txt
-        7z a ${ARCHIVE_FILE} \
-             ./target/${{ matrix.conf }}-pc-windows-msvc/release/oxipng.exe \
-             ./CHANGELOG.md ./LICENSE.txt ./README.md
-        echo "::set-output name=file::${ARCHIVE_FILE}"
-        echo "::set-output name=name::${ARCHIVE_NAME}.zip"
-
-    - name: Upload artifacts
-      uses: actions/upload-artifact@v2
-      with:
-        name: ${{ steps.package.outputs.name }}
-        path: ${{ steps.package.outputs.file }}
+      - uses: actions/checkout@v3
+        with:
+          persist-credentials: false
+
+      - name: Install stable
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          target: ${{ matrix.conf }}-pc-windows-msvc
+          override: true
+
+      - name: Build oxipng
+        run: |
+          cargo build --release --target ${{ matrix.conf }}-pc-windows-msvc
+
+      - name: Get the version
+        shell: bash
+        id: tagName
+        run: |
+          VERSION=$(cargo pkgid | cut -d# -f2 | cut -d: -f2)
+          echo "::set-output name=tag::$VERSION"
+
+      - name: Build package
+        id: package
+        shell: bash
+        run: |
+          ARCHIVE_TARGET="${{ matrix.conf }}-pc-windows-msvc"
+          ARCHIVE_NAME="oxipng-${{ steps.tagName.outputs.tag }}-$ARCHIVE_TARGET"
+          ARCHIVE_FILE="${ARCHIVE_NAME}.zip"
+          mv LICENSE LICENSE.txt
+          7z a ${ARCHIVE_FILE} \
+               ./target/${{ matrix.conf }}-pc-windows-msvc/release/oxipng.exe \
+               ./CHANGELOG.md ./LICENSE.txt ./README.md
+          echo "::set-output name=file::${ARCHIVE_FILE}"
+          echo "::set-output name=name::${ARCHIVE_NAME}.zip"
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        with:
+          name: ${{ steps.package.outputs.name }}
+          path: ${{ steps.package.outputs.file }}
 
   create-unix-binaries:
+    runs-on: ${{ matrix.os }}
+    if: github.repository == 'shssoichiro/oxipng'
 
     strategy:
       matrix:
@@ -71,67 +74,68 @@ jobs:
           - os: macos-latest
             target: x86_64-apple-darwin
 
-    runs-on: ${{ matrix.os }}
-
     steps:
-    - uses: actions/checkout@v2
-
-    - name: Install Rust
-      uses: actions-rs/toolchain@v1
-      with:
-        profile: minimal
-        toolchain: stable
-        target: ${{ matrix.target }}
-        override: true
-
-    - name: Install musl
-      if: contains(matrix.target, 'linux-musl')
-      run: |
-        sudo apt-get install musl-tools
-
-    - name: Build oxipng
-      run: |
-        cargo build --release --target ${{ matrix.target }}
-
-    - name: Strip binary
-      run: |
-        strip target/${{ matrix.target }}/release/oxipng
-
-    - name: Get the version
-      id: tagName
-      run: |
-        VERSION=$(cargo pkgid | cut -d# -f2 | cut -d: -f2)
-        echo "::set-output name=tag::$VERSION"
-
-    - name: Build package
-      id: package
-      run: |
-        ARCHIVE_TARGET=${{ matrix.target }}
-        ARCHIVE_NAME="oxipng-${{ steps.tagName.outputs.tag }}-$ARCHIVE_TARGET"
-        ARCHIVE_FILE="${ARCHIVE_NAME}.tar.gz"
-        mkdir "/tmp/${ARCHIVE_NAME}"
-        cp README.md CHANGELOG.md LICENSE \
-           target/${{ matrix.target }}/release/oxipng \
-           /tmp/${ARCHIVE_NAME}
-        tar -czf ${PWD}/${ARCHIVE_FILE} -C /tmp/ ${ARCHIVE_NAME}
-        echo ::set-output "name=file::${ARCHIVE_FILE}"
-        echo ::set-output "name=name::${ARCHIVE_NAME}.tar.gz"
-
-    - name: Upload artifacts
-      uses: actions/upload-artifact@v2
-      with:
-        name: ${{ steps.package.outputs.name }}
-        path: ${{ steps.package.outputs.file }}
+      - uses: actions/checkout@v3
+        with:
+          persist-credentials: false
 
+      - name: Install Rust
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          target: ${{ matrix.target }}
+          override: true
 
-  deploy:
+      - name: Install musl
+        if: contains(matrix.target, 'linux-musl')
+        run: |
+          sudo apt-get install musl-tools
+
+      - name: Build oxipng
+        run: |
+          cargo build --release --target ${{ matrix.target }}
+
+      - name: Strip binary
+        run: |
+          strip target/${{ matrix.target }}/release/oxipng
+
+      - name: Get the version
+        id: tagName
+        run: |
+          VERSION=$(cargo pkgid | cut -d# -f2 | cut -d: -f2)
+          echo "::set-output name=tag::$VERSION"
+
+      - name: Build package
+        id: package
+        run: |
+          ARCHIVE_TARGET=${{ matrix.target }}
+          ARCHIVE_NAME="oxipng-${{ steps.tagName.outputs.tag }}-$ARCHIVE_TARGET"
+          ARCHIVE_FILE="${ARCHIVE_NAME}.tar.gz"
+          mkdir "/tmp/${ARCHIVE_NAME}"
+          cp README.md CHANGELOG.md LICENSE \
+             target/${{ matrix.target }}/release/oxipng \
+             /tmp/${ARCHIVE_NAME}
+          tar -czf ${PWD}/${ARCHIVE_FILE} -C /tmp/ ${ARCHIVE_NAME}
+          echo ::set-output "name=file::${ARCHIVE_FILE}"
+          echo ::set-output "name=name::${ARCHIVE_NAME}.tar.gz"
+
+      - name: Upload artifacts
+        uses: actions/upload-artifact@v3
+        with:
+          name: ${{ steps.package.outputs.name }}
+          path: ${{ steps.package.outputs.file }}
 
-    needs: [create-windows-binaries, create-unix-binaries]
 
+  deploy:
     runs-on: ubuntu-latest
+    if: github.repository == 'shssoichiro/oxipng'
+    needs: [create-windows-binaries, create-unix-binaries]
 
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
+        with:
+          persist-credentials: false
 
       - name: Get version and release description
         id: tagName
@@ -141,7 +145,7 @@ jobs:
           echo "::set-output name=tag::$VERSION"
 
       - name: Download artifacts
-        uses: actions/download-artifact@v2
+        uses: actions/download-artifact@v3
         with:
           path: ./binaries
 
diff --git a/.github/workflows/oxipng.yml b/.github/workflows/oxipng.yml
index dfdb48bf..eb0edabf 100644
--- a/.github/workflows/oxipng.yml
+++ b/.github/workflows/oxipng.yml
@@ -7,10 +7,13 @@ on:
   pull_request:
     branches:
       - master
+  workflow_dispatch:
 
 jobs:
   test:
+    runs-on: ${{ matrix.os }}
     strategy:
+      fail-fast: false
       matrix:
         target:
           - x86_64-unknown-linux-gnu
@@ -45,12 +48,25 @@ jobs:
             toolchain: beta
           - target: x86_64-unknown-linux-musl
             toolchain: nightly
-    runs-on: ${{ matrix.os }}
+
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
+        with:
+          persist-credentials: false
       - name: Install musl tools
         run: sudo apt-get install musl-tools
         if: "contains(matrix.target, 'musl')"
+      - name: Cache cargo registry
+        uses: actions/cache@v3
+        with:
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+          key: ${{ runner.os }}-${{ matrix.toolchain }}-cargo-registry-${{ hashFiles('**/Cargo.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-${{ matrix.toolchain }}-cargo-registry-
       - name: Install ${{ matrix.toolchain }}
         uses: actions-rs/toolchain@v1
         with:
@@ -58,13 +74,6 @@ jobs:
           toolchain: ${{ matrix.toolchain }}
           override: true
           components: clippy, rustfmt
-      - name: Cache cargo registry
-        uses: actions/cache@v1
-        with:
-          path: ~/.cargo/registry/cache
-          key: ${{ matrix.target }}-${{ matrix.toolchain }}-cargo-registry-${{ hashFiles('**/Cargo.lock') }}
-          restore-keys: |
-            ${{ matrix.target }}-${{ matrix.toolchain }}-cargo-registry-
       - name: Run rustfmt
         if: matrix.toolchain == 'stable'
         uses: actions-rs/cargo@v1
@@ -78,7 +87,7 @@ jobs:
           token: ${{ secrets.GITHUB_TOKEN }}
           args: -- -D warnings
       - name: Run tests
-        run: cargo test
+        run: cargo test --features sanity-checks
       - name: Build benchmarks
         if: matrix.toolchain == 'nightly'
         run: cargo bench --no-run
diff --git a/Cargo.toml b/Cargo.toml
index 8f4a214d..443f81d1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -22,6 +22,10 @@ name = "oxipng"
 path = "src/main.rs"
 required-features = ["binary"]
 
+[[bench]]
+name = "zopfli"
+required-features = ["zopfli"]
+
 [dependencies]
 zopfli = { version = "0.7.1", optional = true }
 rgb = "0.8.33"
@@ -53,6 +57,7 @@ optional = true
 version = "2.1.0"
 
 [dependencies.image]
+optional = true
 default-features = false
 features = ["png"]
 version = "0.24.3"
@@ -65,6 +70,7 @@ binary = ["clap", "wild", "stderrlog"]
 default = ["binary", "filetime", "parallel", "zopfli"]
 parallel = ["rayon", "indexmap/rayon", "crossbeam-channel"]
 freestanding = ["libdeflater/freestanding"]
+sanity-checks = ["image"]
 
 [lib]
 name = "oxipng"
diff --git a/src/lib.rs b/src/lib.rs
index 614246ce..0f5b637f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -31,12 +31,11 @@ use crate::evaluate::Evaluator;
 use crate::png::PngData;
 use crate::png::PngImage;
 use crate::reduction::*;
-use image::{DynamicImage, GenericImageView, ImageFormat, Pixel};
-use log::{debug, error, info, warn};
+use log::{debug, info, warn};
 use rayon::prelude::*;
 use std::fmt;
 use std::fs::{copy, File, Metadata};
-use std::io::{stdin, stdout, BufWriter, Cursor, Read, Write};
+use std::io::{stdin, stdout, BufWriter, Read, Write};
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
@@ -659,7 +658,8 @@ fn optimize_png(
         );
     }
 
-    debug_assert!(validate_output(&output, original_data));
+    #[cfg(feature = "sanity-checks")]
+    debug_assert!(sanity_checks::validate_output(&output, original_data));
 
     Ok(output)
 }
@@ -1044,46 +1044,54 @@ fn copy_times(input_path_meta: &Metadata, out_path: &Path) -> PngResult<()> {
     })
 }
 
-/// Validate that the output png data still matches the original image
-fn validate_output(output: &[u8], original_data: &[u8]) -> bool {
-    let (old_png, new_png) = rayon::join(
-        || load_png_image_from_memory(original_data),
-        || load_png_image_from_memory(output),
-    );
+#[cfg(feature = "sanity-checks")]
+mod sanity_checks {
+    use super::*;
+    use image::{DynamicImage, GenericImageView, ImageFormat, Pixel};
+    use log::error;
+    use std::io::Cursor;
+
+    /// Validate that the output png data still matches the original image
+    pub(super) fn validate_output(output: &[u8], original_data: &[u8]) -> bool {
+        let (old_png, new_png) = rayon::join(
+            || load_png_image_from_memory(original_data),
+            || load_png_image_from_memory(output),
+        );
 
-    match (new_png, old_png) {
-        (Err(new_err), _) => {
-            error!("Failed to read output image for validation: {}", new_err);
-            false
-        }
-        (_, Err(old_err)) => {
-            // The original image might be invalid if, for example, there is a CRC error,
-            // and we set fix_errors to true. In that case, all we can do is check that the
-            // new image is decodable.
-            warn!("Failed to read input image for validation: {}", old_err);
-            true
+        match (new_png, old_png) {
+            (Err(new_err), _) => {
+                error!("Failed to read output image for validation: {}", new_err);
+                false
+            }
+            (_, Err(old_err)) => {
+                // The original image might be invalid if, for example, there is a CRC error,
+                // and we set fix_errors to true. In that case, all we can do is check that the
+                // new image is decodable.
+                warn!("Failed to read input image for validation: {}", old_err);
+                true
+            }
+            (Ok(new_png), Ok(old_png)) => images_equal(&old_png, &new_png),
         }
-        (Ok(new_png), Ok(old_png)) => images_equal(&old_png, &new_png),
     }
-}
 
-/// Loads a PNG image from memory to a [DynamicImage]
-fn load_png_image_from_memory(png_data: &[u8]) -> Result<DynamicImage, image::ImageError> {
-    let mut reader = image::io::Reader::new(Cursor::new(png_data));
-    reader.set_format(ImageFormat::Png);
-    reader.no_limits();
-    reader.decode()
-}
+    /// Loads a PNG image from memory to a [DynamicImage]
+    fn load_png_image_from_memory(png_data: &[u8]) -> Result<DynamicImage, image::ImageError> {
+        let mut reader = image::io::Reader::new(Cursor::new(png_data));
+        reader.set_format(ImageFormat::Png);
+        reader.no_limits();
+        reader.decode()
+    }
 
-/// Compares images pixel by pixel for equivalent content
-fn images_equal(old_png: &DynamicImage, new_png: &DynamicImage) -> bool {
-    let a = old_png.pixels().filter(|x| {
-        let p = x.2.channels();
-        !(p.len() == 4 && p[3] == 0)
-    });
-    let b = new_png.pixels().filter(|x| {
-        let p = x.2.channels();
-        !(p.len() == 4 && p[3] == 0)
-    });
-    a.eq(b)
+    /// Compares images pixel by pixel for equivalent content
+    fn images_equal(old_png: &DynamicImage, new_png: &DynamicImage) -> bool {
+        let a = old_png.pixels().filter(|x| {
+            let p = x.2.channels();
+            !(p.len() == 4 && p[3] == 0)
+        });
+        let b = new_png.pixels().filter(|x| {
+            let p = x.2.channels();
+            !(p.len() == 4 && p[3] == 0)
+        });
+        a.eq(b)
+    }
 }
diff --git a/src/rayon.rs b/src/rayon.rs
index 768db04c..ae1914c2 100644
--- a/src/rayon.rs
+++ b/src/rayon.rs
@@ -52,6 +52,7 @@ where
 
 impl<I: Iterator> ParallelIterator for I {}
 
+#[allow(dead_code)]
 pub fn join<A, B>(a: impl FnOnce() -> A, b: impl FnOnce() -> B) -> (A, B) {
     (a(), b())
 }


Test patch:
diff --git a/tests/flags.rs b/tests/flags.rs
index 38db85b7..fc7ace9e 100644
--- a/tests/flags.rs
+++ b/tests/flags.rs
@@ -27,6 +27,7 @@ fn get_opts(input: &Path) -> (OutFile, oxipng::Options) {
 }
 
 /// Add callback to allow checks before the output file is deleted again
+#[allow(clippy::too_many_arguments)]
 fn test_it_converts_callbacks<CBPRE, CBPOST>(
     input: PathBuf,
     output: &OutFile,
@@ -38,8 +39,8 @@ fn test_it_converts_callbacks<CBPRE, CBPOST>(
     mut callback_pre: CBPRE,
     mut callback_post: CBPOST,
 ) where
-    CBPOST: FnMut(&Path) -> (),
-    CBPRE: FnMut(&Path) -> (),
+    CBPOST: FnMut(&Path),
+    CBPRE: FnMut(&Path),
 {
     let png = PngData::new(&input, opts.fix_errors).unwrap();
 
@@ -48,14 +49,14 @@ fn test_it_converts_callbacks<CBPRE, CBPOST>(
 
     callback_pre(&input);
 
-    match oxipng::optimize(&InFile::Path(input), &output, &opts) {
+    match oxipng::optimize(&InFile::Path(input), output, opts) {
         Ok(_) => (),
         Err(x) => panic!("{}", x),
     };
     let output = output.path().unwrap();
     assert!(output.exists());
 
-    callback_post(&output);
+    callback_post(output);
 
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
diff --git a/tests/reduction.rs b/tests/reduction.rs
index f3afdb88..d88cc58c 100644
--- a/tests/reduction.rs
+++ b/tests/reduction.rs
@@ -834,7 +834,7 @@ fn small_files() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };
@@ -866,7 +866,7 @@ fn palette_should_be_reduced_with_dupes() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };
@@ -899,7 +899,7 @@ fn palette_should_be_reduced_with_unused() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };
@@ -932,7 +932,7 @@ fn palette_should_be_reduced_with_both() {
     let png = match PngData::new(output, opts.fix_errors) {
         Ok(x) => x,
         Err(x) => {
-            remove_file(&output).ok();
+            remove_file(output).ok();
             panic!("{}", x)
         }
     };