Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions .github/workflows/ci.yaml

This file was deleted.

147 changes: 147 additions & 0 deletions .github/workflows/release.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
name: Release

on:
release:
types:
- created
push:
tags:
- "v*.*.*"
# push:
# branches:
# - 'main'
# pull_request:
# branches:
# - '**'

jobs:
build:
runs-on: ubuntu-20.04

steps:
- name: Checkout code
uses: actions/checkout@v3
Comment on lines +22 to +23
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update GitHub Actions versions and deprecated syntax.

Several updates needed:

  1. Update actions to latest versions
  2. Replace deprecated set-output command
-      uses: actions/checkout@v3
+      uses: actions/checkout@v4

-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4

-      run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"
+      run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT

-      uses: softprops/action-gh-release@v1
+      uses: softprops/action-gh-release@v2

Also applies to: 110-112, 117-119, 122-124

🧰 Tools
🪛 actionlint (1.7.4)

23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 yamllint (1.35.1)

[warning] 22-22: wrong indentation: expected 6 but found 4

(indentation)


- name: Checkout BMF repository (specific branch)
run: |
sudo apt update
sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
# sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa

git clone https://github.com/JackLau1222/bmf.git

Comment on lines +25 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use repository variables for external dependencies.

Move the hardcoded BMF repository URL to repository variables for better maintainability and security.

     - name: Checkout BMF repository (specific branch)
+      env:
+        BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }}
+        BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }}
       run: |
         sudo apt update
         sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
         sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
-        git clone https://github.com/JackLau1222/bmf.git
+        git clone "$BMF_REPO" bmf
+        cd bmf
+        git checkout "$BMF_BRANCH"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Checkout BMF repository (specific branch)
run: |
sudo apt update
sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
# sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa
git clone https://github.com/JackLau1222/bmf.git
- name: Checkout BMF repository (specific branch)
env:
BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }}
BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }}
run: |
sudo apt update
sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
# sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa
git clone "$BMF_REPO" bmf
cd bmf
git checkout "$BMF_BRANCH"
🧰 Tools
🪛 yamllint (1.35.1)

[error] 31-31: trailing spaces

(trailing-spaces)

# - name: Cache FFmpeg build
# uses: actions/cache@v3
# with:
# path: |
# /usr/local/bin/ffmpeg
# /usr/local/lib/
# /usr/local/include/
# key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
# restore-keys: |
# ${{ runner.os }}-ffmpeg-

# - name: Cache BMF build
# uses: actions/cache@v3
# with:
# path: bmf/output/
# key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
# restore-keys: |
# ${{ runner.os }}-bmf-
Comment on lines +34 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enable and update build caching.

Enable build caching with the latest cache action version to improve workflow performance.

-    # - name: Cache FFmpeg build
-    #   uses: actions/cache@v3
+    - name: Cache FFmpeg build
+      uses: actions/cache@v4
       with:
         path: |
           /usr/local/bin/ffmpeg
           /usr/local/lib/
           /usr/local/include/
         key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
         restore-keys: |
           ${{ runner.os }}-ffmpeg-

-    # - name: Cache BMF build
-    #   uses: actions/cache@v3
+    - name: Cache BMF build
+      uses: actions/cache@v4
       with:
         path: bmf/output/
         key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
         restore-keys: |
           ${{ runner.os }}-bmf-
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# - name: Cache FFmpeg build
# uses: actions/cache@v3
# with:
# path: |
# /usr/local/bin/ffmpeg
# /usr/local/lib/
# /usr/local/include/
# key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
# restore-keys: |
# ${{ runner.os }}-ffmpeg-
# - name: Cache BMF build
# uses: actions/cache@v3
# with:
# path: bmf/output/
# key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
# restore-keys: |
# ${{ runner.os }}-bmf-
- name: Cache FFmpeg build
uses: actions/cache@v4
with:
path: |
/usr/local/bin/ffmpeg
/usr/local/lib/
/usr/local/include/
key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
restore-keys: |
${{ runner.os }}-ffmpeg-
- name: Cache BMF build
uses: actions/cache@v4
with:
path: bmf/output/
key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
restore-keys: |
${{ runner.os }}-bmf-


- name: Compile FFmpeg and BMF if not cached
run: |
if [ ! -f "/usr/local/bin/ffmpeg" ]; then
echo "FFmpeg not found, starting build..."
(cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
else
echo "FFmpeg is already installed, skipping build."
fi

- name: Set up BMF
run: |
cd bmf
echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
Comment on lines +62 to +68
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix environment variable setup.

The current environment variable setup has shell scripting issues and could be more maintainable.

     - name: Set up BMF
       run: |
         cd bmf
-        echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
-        echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
-        echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
-        echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
+        {
+          echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include"
+          echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include"
+          echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib"
+          echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib"
+        } >> "$GITHUB_ENV"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Set up BMF
run: |
cd bmf
echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
- name: Set up BMF
run: |
cd bmf
{
echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include"
echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include"
echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib"
echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib"
} >> "$GITHUB_ENV"
🧰 Tools
🪛 actionlint (1.7.4)

63-63: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects

(shellcheck)


63-63: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting

(shellcheck)


63-63: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting

(shellcheck)


63-63: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting

(shellcheck)


63-63: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting

(shellcheck)



- name: Set up Qt
run: |
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential

- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
Comment on lines +75 to +77
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix build command substitution.

The build command needs proper quoting to prevent word splitting.

     - name: Build with CMake
       run: |
-        (cd src && cmake -B build && cd build && make -j$(nproc))
+        cd src
+        cmake -B build
+        cd build
+        make -j"$(nproc)"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
- name: Build with CMake
run: |
cd src
cmake -B build
cd build
make -j"$(nproc)"
🧰 Tools
🪛 actionlint (1.7.4)

76-76: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting

(shellcheck)


Comment on lines +71 to +78
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve Qt setup and build reliability.

  1. Pin Qt package versions for reproducibility
  2. Add error handling for build steps
  3. Fix command substitution in make command

Apply this diff:

     - name: Set up Qt
       run: |
+        set -eo pipefail
         sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential

     - name: Build with CMake
       run: |
+        set -eo pipefail
-        (cd src && cmake -B build && cd build && make -j$(nproc))
+        cd src
+        cmake -B build
+        cd build
+        make -j"$(nproc)"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Set up Qt
run: |
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
- name: Set up Qt
run: |
set -eo pipefail
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
- name: Build with CMake
run: |
set -eo pipefail
cd src
cmake -B build
cd build
make -j"$(nproc)"
🧰 Tools
🪛 actionlint (1.7.4)

63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting

(shellcheck)

🪛 yamllint (1.35.1)

[error] 65-65: trailing spaces

(trailing-spaces)

# - name: Start tmate session for debugging
# uses: mxschmitt/action-tmate@v3
# with:
# sudo: true
# duration: 7200

Comment on lines +71 to +84
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance build environment setup and performance

Consider these improvements:

  1. Pin Qt version for reproducibility
  2. Remove debugging configuration from production workflow
  3. Add build caching to speed up workflows
+    - name: Cache Qt
+      uses: actions/cache@v3
+      with:
+        path: |
+          /usr/lib/qt5
+          ~/.cache/pip
+        key: ${{ runner.os }}-qt5-${{ hashFiles('**/CMakeLists.txt') }}
+
     - name: Set up Qt
       run: |
-        sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
+        sudo apt-get install -y \
+          qt5-qmake=5.12.8* \
+          qtbase5-dev=5.12.8* \
+          qtchooser=66-2* \
+          qtbase5-dev-tools=5.12.8* \
+          cmake \
+          build-essential

-    # - name: Start tmate session for debugging
-    #   uses: mxschmitt/action-tmate@v3
-    #   with:
-    #     sudo: true
-    #     duration: 7200
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Set up Qt
run: |
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
# - name: Start tmate session for debugging
# uses: mxschmitt/action-tmate@v3
# with:
# sudo: true
# duration: 7200
- name: Cache Qt
uses: actions/cache@v3
with:
path: |
/usr/lib/qt5
~/.cache/pip
key: ${{ runner.os }}-qt5-${{ hashFiles('**/CMakeLists.txt') }}
- name: Set up Qt
run: |
sudo apt-get install -y \
qt5-qmake=5.12.8* \
qtbase5-dev=5.12.8* \
qtchooser=66-2* \
qtbase5-dev-tools=5.12.8* \
cmake \
build-essential
- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
🧰 Tools
🪛 actionlint (1.7.4)

63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting

(shellcheck)

🪛 yamllint (1.35.1)

[error] 65-65: trailing spaces

(trailing-spaces)

- name: Copy libs
run: |
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
# Then build in Qt Creator, or use
export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
(linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
continue-on-error: true
Comment on lines +85 to +101
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve AppImage creation reliability.

Remove continue-on-error and add proper error handling for the AppImage creation process.

     - name: Copy libs
       run: |
+        set -eo pipefail
         # linuxdeployqt
         sudo apt-get -y install git g++ libgl1-mesa-dev
         git clone https://github.com/probonopd/linuxdeployqt.git
-        # Then build in Qt Creator, or use
-        export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
+        
+        # Set Qt path safely
+        qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true
+        if [ -n "$qt_path" ]; then
+          export PATH="$qt_path:$PATH"
+        fi
+        
         (cd linuxdeployqt && qmake && make && sudo make install)
         # patchelf
         wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
         tar xf patchelf-0.9.tar.bz2
         ( cd patchelf-0.9/ && ./configure  && make && sudo make install )
         # appimage
         sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
         sudo chmod a+x /usr/local/bin/appimagetool
-        (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
-      continue-on-error: true
+        linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Copy libs
run: |
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
# Then build in Qt Creator, or use
export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
(linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
continue-on-error: true
- name: Copy libs
run: |
set -eo pipefail
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
# Set Qt path safely
qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true
if [ -n "$qt_path" ]; then
export PATH="$qt_path:$PATH"
fi
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage
🧰 Tools
🪛 actionlint (1.7.4)

86-86: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values

(shellcheck)


Comment on lines +85 to +102
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix critical issues in library setup.

Several critical issues need attention:

  1. Remove continue-on-error from AppImage creation
  2. Add error handling
  3. Fix PATH manipulation
  4. Verify dependencies before proceeding

Apply this diff:

     - name: Copy libs
       run: |
+        set -eo pipefail
         # linuxdeployqt
         sudo apt-get -y install git g++ libgl1-mesa-dev
+        
+        # Verify Qt installation
+        if ! command -v qmake &> /dev/null; then
+          echo "Error: Qt installation not found"
+          exit 1
+        fi
+        
         git clone https://github.com/probonopd/linuxdeployqt.git
-        # Then build in Qt Creator, or use
-        export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
+        
+        # Set Qt path safely
+        qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true
+        if [ -n "$qt_path" ]; then
+          export PATH="$qt_path:$PATH"
+        fi
+        
         (cd linuxdeployqt && qmake && make && sudo make install)
+        
         # patchelf
         wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
         tar xf patchelf-0.9.tar.bz2
         ( cd patchelf-0.9/ && ./configure  && make && sudo make install )
+        
         # appimage
         sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
         sudo chmod a+x /usr/local/bin/appimagetool
-        (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
-      continue-on-error: true
+        linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Copy libs
run: |
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
# Then build in Qt Creator, or use
export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
(linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
continue-on-error: true
- name: Copy libs
run: |
set -eo pipefail
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
# Verify Qt installation
if ! command -v qmake &> /dev/null; then
echo "Error: Qt installation not found"
exit 1
fi
git clone https://github.com/probonopd/linuxdeployqt.git
# Set Qt path safely
qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true
if [ -n "$qt_path" ]; then
export PATH="$qt_path:$PATH"
fi
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage
🧰 Tools
🪛 actionlint (1.7.4)

73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values

(shellcheck)

🪛 yamllint (1.35.1)

[error] 89-89: trailing spaces

(trailing-spaces)


- name: Copy runtime
run: |
# mkdir /src/build/lib
cp /usr/local/lib/libswscale.so.6 src/build/lib
cp /usr/local/lib/libavfilter.so.8 src/build/lib
cp /usr/local/lib/libpostproc.so.56 src/build/lib
cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib
cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
touch src/build/activate_env.sh
echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh
Comment on lines +104 to +113
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve runtime library management.

Add error handling and verification for library copying operations.

     - name: Copy runtime
       run: |
+        set -eo pipefail
-        # mkdir /src/build/lib
+        mkdir -p src/build/lib
+        
+        # Verify libraries exist before copying
+        LIBS=(
+          "/usr/local/lib/libswscale.so.6"
+          "/usr/local/lib/libavfilter.so.8"
+          "/usr/local/lib/libpostproc.so.56"
+          "bmf/output/bmf/lib/libbuiltin_modules.so"
+        )
+        
+        for lib in "${LIBS[@]}"; do
+          if [ ! -f "$lib" ]; then
+            echo "Error: Required library $lib not found"
+            exit 1
+          fi
+          cp "$lib" src/build/lib/
+        done
+        
+        if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then
+          echo "Error: BUILTIN_CONFIG.json not found"
+          exit 1
+        fi
         cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
-        touch src/build/activate_env.sh
-        echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh
+        
+        cat > src/build/activate_env.sh << 'EOF'
+        #!/bin/bash
+        BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+        export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
+        EOF
+        chmod +x src/build/activate_env.sh
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Copy runtime
run: |
# mkdir /src/build/lib
cp /usr/local/lib/libswscale.so.6 src/build/lib
cp /usr/local/lib/libavfilter.so.8 src/build/lib
cp /usr/local/lib/libpostproc.so.56 src/build/lib
cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib
cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
touch src/build/activate_env.sh
echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh
- name: Copy runtime
run: |
set -eo pipefail
mkdir -p src/build/lib
# Verify libraries exist before copying
LIBS=(
"/usr/local/lib/libswscale.so.6"
"/usr/local/lib/libavfilter.so.8"
"/usr/local/lib/libpostproc.so.56"
"bmf/output/bmf/lib/libbuiltin_modules.so"
)
for lib in "${LIBS[@]}"; do
if [ ! -f "$lib" ]; then
echo "Error: Required library $lib not found"
exit 1
fi
cp "$lib" src/build/lib/
done
if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then
echo "Error: BUILTIN_CONFIG.json not found"
exit 1
fi
cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
cat > src/build/activate_env.sh << 'EOF'
#!/bin/bash
BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
EOF
chmod +x src/build/activate_env.sh


# Step to package the build directory
- name: Create tar.gz package
run: |
# VERSION="1.3"
BUILD_DIR="src/build"
PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz"
OUTPUT_DIR="OpenConverter_Linux_x86"
mkdir -p $OUTPUT_DIR
cp -r $BUILD_DIR/* $OUTPUT_DIR/
tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .
rm -rf $OUTPUT_DIR

# Step to upload the tar.gz package as an artifact
- name: Upload build artifact
uses: actions/upload-artifact@v3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update deprecated syntax and actions.

Several updates are needed:

  1. Replace deprecated set-output command
  2. Update GitHub Actions to latest versions
  3. Fix release asset path
-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4

-      run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"
+      run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT

-      uses: softprops/action-gh-release@v1
+      uses: softprops/action-gh-release@v2
       with:
-        files: ./build/OpenConverter_Linux_x86.tar.gz
+        files: ./OpenConverter_Linux_x86.tar.gz

Also applies to: 137-137, 141-141

🧰 Tools
🪛 actionlint (1.7.4)

129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

with:
name: OpenConverter_Linux_x86
path: OpenConverter_Linux_x86.tar.gz


- name: Get GitHub Release information
id: release_info
run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"


- name: Upload Release Asset
uses: softprops/action-gh-release@v1
if: startsWith(github.ref, 'refs/tags/')
with:
files: ./build/OpenConverter_Linux_x86.tar.gz

- name: Finish
run: echo "Release upload complete"
Comment on lines +135 to +147
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update deprecated syntax and fix path inconsistencies

Several updates are needed:

  1. Replace deprecated set-output command
  2. Fix path mismatch in release asset upload
  3. Update GitHub Actions to latest versions
     - name: Get GitHub Release information
       id: release_info
-      run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"
+      run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT

     - name: Upload Release Asset
-      uses: softprops/action-gh-release@v1
+      uses: softprops/action-gh-release@v2
       if: startsWith(github.ref, 'refs/tags/')
       with:
-        files: ./build/OpenConverter_Linux_x86.tar.gz
+        files: ./OpenConverter_Linux_x86.tar.gz

Also update other action versions:

     - name: Checkout code
-      uses: actions/checkout@v3
+      uses: actions/checkout@v4

     - name: Upload build artifact
-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 actionlint (1.7.4)

117-117: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'

(shellcheck)


117-117: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting

(shellcheck)


117-117: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions

(deprecated-commands)


121-121: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 yamllint (1.35.1)

[error] 127-127: no new line character at the end of file

(new-line-at-end-of-file)

123 changes: 123 additions & 0 deletions .github/workflows/review.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
name: Review

on:
pull_request:
branches:
- '**'

jobs:
build:
runs-on: ubuntu-20.04

steps:
- name: Checkout code
uses: actions/checkout@v3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update checkout action version

The checkout action version should be updated to v4 for better compatibility and security.

-      uses: actions/checkout@v3
+      uses: actions/checkout@v4
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uses: actions/checkout@v3
uses: actions/checkout@v4
🧰 Tools
🪛 actionlint (1.7.4)

14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


- name: Checkout BMF repository (specific branch)
run: |
sudo apt update
sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
# sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa

git clone https://github.com/JackLau1222/bmf.git

# - name: Cache FFmpeg build
# uses: actions/cache@v3
# with:
# path: |
# /usr/local/bin/ffmpeg
# /usr/local/lib/libav*
# /usr/local/lib/libsw*
# /usr/local/lib/libpostproc*
# /usr/local/include/libav*
# /usr/local/include/libsw*
# /usr/local/include/libpostproc*
# key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
# restore-keys: |
# ${{ runner.os }}-ffmpeg-

# - name: Cache BMF build
# uses: actions/cache@v3
# with:
# path: bmf/output/
# key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
# restore-keys: |
# ${{ runner.os }}-bmf-

- name: Compile FFmpeg and BMF if not cached
run: |
if [ ! -f "/usr/local/bin/ffmpeg" ]; then
echo "FFmpeg not found, starting build..."
(cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
else
echo "FFmpeg is already installed, skipping build."
fi
Comment on lines +50 to +55
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for BMF build.

The BMF build step lacks proper error handling. If the build fails, the workflow continues without proper error reporting.

-        if [ ! -f "/usr/local/bin/ffmpeg" ]; then
-          echo "FFmpeg not found, starting build..."
-          (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
-        else
-          echo "FFmpeg is already installed, skipping build."
-        fi
+        if [ ! -f "/usr/local/bin/ffmpeg" ]; then
+          echo "FFmpeg not found, starting build..."
+          (
+            cd bmf || exit 1
+            git checkout fork_by_oc || exit 1
+            if ! sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus; then
+              echo "::error::FFmpeg build failed"
+              exit 1
+            fi
+            if ! ./build.sh; then
+              echo "::error::BMF build failed"
+              exit 1
+            fi
+          )
+        else
+          echo "FFmpeg is already installed, skipping build."
+        fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [ ! -f "/usr/local/bin/ffmpeg" ]; then
echo "FFmpeg not found, starting build..."
(cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
else
echo "FFmpeg is already installed, skipping build."
fi
if [ ! -f "/usr/local/bin/ffmpeg" ]; then
echo "FFmpeg not found, starting build..."
(
cd bmf || exit 1
git checkout fork_by_oc || exit 1
if ! sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus; then
echo "::error::FFmpeg build failed"
exit 1
fi
if ! ./build.sh; then
echo "::error::BMF build failed"
exit 1
fi
)
else
echo "FFmpeg is already installed, skipping build."
fi


- name: Set up BMF
run: |
cd bmf
echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
Comment on lines +60 to +63
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix environment variable concatenation

The environment variables should be properly quoted to prevent word splitting:

-        echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
-        echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
-        echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
-        echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
+        {
+          echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+"$C_INCLUDE_PATH:"}$(pwd)/output/bmf/include"
+          echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+"$CPLUS_INCLUDE_PATH:"}$(pwd)/output/bmf/include"
+          echo "LIBRARY_PATH=${LIBRARY_PATH:+"$LIBRARY_PATH:"}$(pwd)/output/bmf/lib"
+          echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+"$LD_LIBRARY_PATH:"}$(pwd)/output/bmf/lib"
+        } >> "$GITHUB_ENV"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
{
echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+"$C_INCLUDE_PATH:"}$(pwd)/output/bmf/include"
echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+"$CPLUS_INCLUDE_PATH:"}$(pwd)/output/bmf/include"
echo "LIBRARY_PATH=${LIBRARY_PATH:+"$LIBRARY_PATH:"}$(pwd)/output/bmf/lib"
echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+"$LD_LIBRARY_PATH:"}$(pwd)/output/bmf/lib"
} >> "$GITHUB_ENV"



- name: Set up Qt
run: |
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove unnecessary qmake installation

Since the project is moving to CMake, qmake-related packages are no longer needed.

-        sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
+        sudo apt-get install -y qtbase5-dev cmake build-essential
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
sudo apt-get install -y qtbase5-dev cmake build-essential


- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix CMake build command

The build command needs proper quoting and should use a configurable build directory:

-        (cd src && cmake -B build && cd build && make -j$(nproc))
+        BUILD_DIR="${BUILD_DIR:-build}"
+        (cd src && cmake -B "$BUILD_DIR" && cd "$BUILD_DIR" && make "-j$(nproc)")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
(cd src && cmake -B build && cd build && make -j$(nproc))
BUILD_DIR="${BUILD_DIR:-build}"
(cd src && cmake -B "$BUILD_DIR" && cd "$BUILD_DIR" && make "-j$(nproc)")

Comment on lines +71 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add build validation steps

The workflow should validate the build artifacts before packaging:

     - name: Build with CMake
       run: |
         (cd src && cmake -B build && cd build && make -j$(nproc))
+        # Validate build artifacts
+        if [ ! -f "src/build/OpenConverter" ]; then
+          echo "::error::Build failed: OpenConverter binary not found"
+          exit 1
+        fi

     # ... other steps ...

     - name: Create tar.gz package
       run: |
+        # Validate required files before packaging
+        required_files=("OpenConverter" "lib" "BUILTIN_CONFIG.json" "activate_env.sh")
+        for file in "${required_files[@]}"; do
+          if [ ! -e "$BUILD_DIR/$file" ]; then
+            echo "::error::Required file/directory not found: $file"
+            exit 1
+          fi
+        done
         BUILD_DIR="src/build"

Also applies to: 75-82

🧰 Tools
🪛 actionlint (1.7.4)

40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting

(shellcheck)


- name: Copy libs
run: |
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
# Then build in Qt Creator, or use
export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
Comment on lines +77 to +82
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use pre-built linuxdeployqt

Building linuxdeployqt from source using qmake in a CMake workflow is inconsistent. Consider using pre-built releases:

-        sudo apt-get -y install git g++ libgl1-mesa-dev
-        git clone https://github.com/probonopd/linuxdeployqt.git
-        export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
-        (cd linuxdeployqt && qmake && make && sudo make install)
+        LINUXDEPLOYQT_VERSION="8"
+        wget -c -nv "https://github.com/probonopd/linuxdeployqt/releases/download/${LINUXDEPLOYQT_VERSION}/linuxdeployqt-continuous-x86_64.AppImage"
+        chmod a+x linuxdeployqt-continuous-x86_64.AppImage
+        sudo mv linuxdeployqt-continuous-x86_64.AppImage /usr/local/bin/linuxdeployqt
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
# Then build in Qt Creator, or use
export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
LINUXDEPLOYQT_VERSION="8"
wget -c -nv "https://github.com/probonopd/linuxdeployqt/releases/download/${LINUXDEPLOYQT_VERSION}/linuxdeployqt-continuous-x86_64.AppImage"
chmod a+x linuxdeployqt-continuous-x86_64.AppImage
sudo mv linuxdeployqt-continuous-x86_64.AppImage /usr/local/bin/linuxdeployqt
# patchelf

wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
Comment on lines +83 to +88
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add checksum verification for downloaded binaries

Downloaded binaries should be verified using checksums to ensure integrity and security.

Add SHA256 verification:

+PATCHELF_SHA256="f2aa40a6148cb3b0ca807a1bf836b081793e55ec9e5540a5356d800132be7e0a"
 wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
+echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c
 
+APPIMAGETOOL_SHA256="df3baf5ca5facbecfc2f3fa6713c29ab9cefa8fd4992c78f35b635c9e27cf3e5"
 sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
+echo "$APPIMAGETOOL_SHA256 /usr/local/bin/appimagetool" | sha256sum -c
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
PATCHELF_SHA256="f2aa40a6148cb3b0ca807a1bf836b081793e55ec9e5540a5356d800132be7e0a"
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
APPIMAGETOOL_SHA256="df3baf5ca5facbecfc2f3fa6713c29ab9cefa8fd4992c78f35b635c9e27cf3e5"
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
echo "$APPIMAGETOOL_SHA256 /usr/local/bin/appimagetool" | sha256sum -c
sudo chmod a+x /usr/local/bin/appimagetool

(linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
continue-on-error: true
Comment on lines +89 to +90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove continue-on-error for critical packaging step

The AppImage creation is a critical step and should not silently continue on failure. Remove continue-on-error and add proper error handling:

-        (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
-      continue-on-error: true
+        if ! linuxdeployqt "./src/${BUILD_DIR}/OpenConverter" -appimage; then
+          echo "::error::Failed to create AppImage"
+          exit 1
+        fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
(linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
continue-on-error: true
if ! linuxdeployqt "./src/${BUILD_DIR}/OpenConverter" -appimage; then
echo "::error::Failed to create AppImage"
exit 1
fi



- name: Copy runtime
run: |
# mkdir /src/build/lib
cp /usr/local/lib/libswscale.so.6 src/build/lib
cp /usr/local/lib/libavfilter.so.8 src/build/lib
cp /usr/local/lib/libpostproc.so.56 src/build/lib
cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib
cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
touch src/build/activate_env.sh
echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh

# Step to package the build directory
- name: Create tar.gz package
run: |
BUILD_DIR="src/build"
PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz"
OUTPUT_DIR="OpenConverter_Linux_x86"
mkdir -p $OUTPUT_DIR
cp -r $BUILD_DIR/* $OUTPUT_DIR/
tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .
rm -rf $OUTPUT_DIR
Comment on lines +106 to +113
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling to package creation

The tar.gz package creation lacks proper error handling and validation.

 BUILD_DIR="src/build"
 PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz"
 OUTPUT_DIR="OpenConverter_Linux_x86"
+
+# Validate source directory
+if [ ! -d "$BUILD_DIR" ]; then
+  echo "::error::Build directory not found: $BUILD_DIR"
+  exit 1
+fi
+
 mkdir -p $OUTPUT_DIR
-cp -r $BUILD_DIR/* $OUTPUT_DIR/
+if ! cp -r $BUILD_DIR/* $OUTPUT_DIR/; then
+  echo "::error::Failed to copy build files"
+  exit 1
+fi
+
-tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .
+if ! tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .; then
+  echo "::error::Failed to create package"
+  exit 1
+fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
run: |
BUILD_DIR="src/build"
PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz"
OUTPUT_DIR="OpenConverter_Linux_x86"
mkdir -p $OUTPUT_DIR
cp -r $BUILD_DIR/* $OUTPUT_DIR/
tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .
rm -rf $OUTPUT_DIR
run: |
BUILD_DIR="src/build"
PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz"
OUTPUT_DIR="OpenConverter_Linux_x86"
# Validate source directory
if [ ! -d "$BUILD_DIR" ]; then
echo "::error::Build directory not found: $BUILD_DIR"
exit 1
fi
mkdir -p $OUTPUT_DIR
if ! cp -r $BUILD_DIR/* $OUTPUT_DIR/; then
echo "::error::Failed to copy build files"
exit 1
fi
if ! tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .; then
echo "::error::Failed to create package"
exit 1
fi
rm -rf $OUTPUT_DIR


# Step to upload the tar.gz package as an artifact
- name: Upload build artifact
uses: actions/upload-artifact@v3
with:
name: OpenConverter_Linux_x86
path: OpenConverter_Linux_x86.tar.gz
Comment on lines +117 to +120
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update upload-artifact action version

The upload-artifact action version needs to be updated as per static analysis:

-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uses: actions/upload-artifact@v3
with:
name: OpenConverter_Linux_x86
path: OpenConverter_Linux_x86.tar.gz
uses: actions/upload-artifact@v4
with:
name: OpenConverter_Linux_x86
path: OpenConverter_Linux_x86.tar.gz
🧰 Tools
🪛 actionlint (1.7.4)

81-81: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


- name: Finish
run: echo "Release upload complete"
6 changes: 4 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
open_converter.pro.user
build*
build-*
.vscode
.DS_Store
./src/.DS_Store
./src/.DS_Store
src/CMakeLists.txt.user
src/build*
Loading
Loading