Skip to content

Conversation

@wjrobinson11
Copy link

@wjrobinson11 wjrobinson11 commented Sep 29, 2022

PR Type

Enhancement, Tests


Description

  • Add support for new OS versions

  • Expand test coverage with Docker and CI

  • Update binary detection and extraction logic

  • Improve documentation and changelog entries


Changes walkthrough 📝

Relevant files
Tests
3 files
test_with_docker.rb
Expand test coverage for various OS versions                         
+58/-15 
ci.yml
Add GitHub Actions CI workflow                                                     
+68/-0   
docker-compose-arm.yml
Add Docker Compose file for ARM testing                                   
+44/-0   
Enhancement
20 files
wkhtmltopdf-binary.gemspec
Update gem version and add metadata                                           
+6/-1     
Dockerfile-almalinux_8
Add Dockerfile for AlmaLinux 8                                                     
+5/-0     
Dockerfile-archlinux
Update Archlinux Dockerfile dependencies                                 
+2/-3     
Dockerfile-centos_6
Update CentOS 6 Dockerfile for EOL                                             
+5/-1     
Dockerfile-centos_8
Update CentOS 8 Dockerfile for EOL                                             
+15/-0   
Dockerfile-debian_10_arm
Add Dockerfile for Debian 10 ARM                                                 
+8/-0     
Dockerfile-debian_11
Add Dockerfile for Debian 11                                                         
+8/-0     
Dockerfile-debian_11_arm
Add Dockerfile for Debian 11 ARM                                                 
+8/-0     
Dockerfile-debian_12
Add Dockerfile for Debian 12                                                         
+8/-0     
Dockerfile-debian_12_arm
Add Dockerfile for Debian 12 ARM                                                 
+8/-0     
Dockerfile-debian_13
Add Dockerfile for Debian 13                                                         
+8/-0     
Dockerfile-debian_9
Update Debian 9 Dockerfile for EOL                                             
+6/-1     
Dockerfile-debian_9_arm
Add Dockerfile for Debian 9 ARM                                                   
+13/-0   
Dockerfile-oraclelinux_8
Add Dockerfile for Oracle Linux 8                                               
+5/-0     
Dockerfile-rockylinux_8
Add Dockerfile for Rocky Linux 8                                                 
+5/-0     
Dockerfile-ubuntu_22.04
Update Ubuntu 22.04 Dockerfile                                                     
+1/-1     
Dockerfile-ubuntu_24.04
Add Dockerfile for Ubuntu 24.04                                                   
+8/-0     
Gemfile
Add Gemfile for dependency management                                       
+4/-0     
wkhtmltopdf
Enhance OS detection and binary extraction                             
+59/-22 
docker-compose.yml
Update Docker Compose with new OS versions                             
+44/-3   
Documentation
2 files
CHANGELOG.md
Update changelog with new versions and features                   
+28/-1   
README.md
Update README with testing instructions                                   
+19/-8   

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • natebird and others added 30 commits March 11, 2021 11:43
    Ubuntu 21.04 was released today, and currently this gem errors because it's not supported. I've installed wkhtmltopdf manually on my system using the binary for Focal Fossa (20.04) and it's working fine based on my testing.
    Use the 20.04 binary for 21.04
    unixmonkey and others added 23 commits May 9, 2024 21:45
    Add support for alibaba cloud linux
    Fix binary used for Amazon Linux 2023 AMI
    Update CHANGELOG for version 0.12.6.8
    Update wkhtmltopdf: Support KDE Neon 22.04 and 24.04
    Add support to ubuntu 22.04 and 24.04 in arm64
    @pps-pr-agent
    Copy link

    pps-pr-agent bot commented May 16, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit f54fc99)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Complex OS Detection

    The OS detection logic in the wkhtmltopdf script has become quite complex with multiple nested conditions. This might make it difficult to maintain and debug in the future. Consider refactoring this part to improve readability and maintainability.

    suffix = case RbConfig::CONFIG['host_os']
             when /linux/
               os = `. /etc/os-release 2> /dev/null && echo ${ID}_${VERSION_ID}`.strip
    
               os = 'ubuntu_16.04' if os.start_with?('ubuntu_16.') ||
                                      os.start_with?('ubuntu_17.') ||
                                      os.start_with?('linuxmint_18.')
    
               os = 'ubuntu_18.04' if os.start_with?('ubuntu_18.') ||
                                      os.start_with?('ubuntu_19.') ||
                                      os.start_with?('elementary') ||
                                      os.start_with?('linuxmint_19.') ||
                                      os.start_with?('pop') ||
                                      os.start_with?('zorin')
    
               os = 'ubuntu_20.04' if os.start_with?('ubuntu_20.') ||
                                      os.start_with?('linuxmint_20.')
    
    	         os = 'ubuntu_21.10' if os.start_with?('ubuntu_21.') ||
                                      os.start_with?('linuxmint_21.')
    
               os = 'ubuntu_22.04' if os.start_with?('ubuntu_22.') ||
                                      os.start_with?('ubuntu_24.') ||
                                      os.start_with?('tuxedo_22.') ||
                                      os.start_with?('neon_24.') ||
                                      os.start_with?('neon_22.') ||
                                      os.start_with?('linuxmint_22')
    
               os = 'centos_6' if (os.start_with?('amzn_') && os != 'amzn_2' && os != 'amzn_2023') ||
                                  (os.empty? && File.read('/etc/centos-release').start_with?('CentOS release 6'))
    
               os = 'centos_7' if (os.start_with?('amzn_2') && !os.start_with?('amzn_20')) ||
                                  os.start_with?('rhel_7.')
    
               os = 'centos_8' if os.start_with?('rocky_8') ||
                                  os.start_with?('rhel_8.') ||
                                  os.start_with?('ol_8.') ||
                                  os.start_with?('almalinux_8') ||
                                  os.start_with?('alinux_') ||
                                  os == 'amzn_2023'
    
               os_based_on_debian_9 = os.start_with?('debian_9') ||
                                      os.start_with?('deepin')
               os = 'debian_9' if os_based_on_debian_9
    
               os = 'debian_10' if !os_based_on_debian_9 && os.start_with?('debian_10')
    
               os = 'debian_11' if !os_based_on_debian_9 && os.start_with?('debian_11')
    
               os = 'debian_12' if !os_based_on_debian_9 && os.start_with?('debian_12')
    
               os = 'debian_12' if !os_based_on_debian_9 && os.start_with?('debian_13')
    
               os = 'archlinux' if os.start_with?('arch_') ||
                                   os.start_with?('manjaro_')
    
               "#{os}_#{architecture}"
    Test Coverage

    While the test coverage has been expanded, it's important to ensure that all new OS versions and architectures are properly tested. Verify that all new Dockerfiles and configurations are included in the test suite.

    require 'minitest/autorun'
    
    def macos?
      ENV['RUNNER_OS'] && ENV['RUNNER_OS'] == 'macOS'
    end
    
    def arm?
      ENV['ARM']
    end
    
    class WithDockerTest < Minitest::Test
      SETUP = begin
                `docker-compose build --no-cache` if !macos?
              end
    
      def test_centos_6
        test_on_x86 with: 'centos_6'
      end
    
      def test_centos_7
        test_on_x86 with: 'centos_7'
      end
    
      def test_centos_8
        test_on_x86 with: 'centos_8'
      end
    
      def test_debian_9
        test_on_x86_and_arm with: 'debian_9'
      end
    
      def test_debian_10
        test_on_x86_and_arm with: 'debian_10'
      end
    
      def test_debian_11
        test_on_x86_and_arm with: 'debian_11'
      end
    
      def test_debian_12
        test_on_x86_and_arm with: 'debian_12'
      end
    
      def test_debian_13
        test_on_x86 with: 'debian_13'
      end
    
      def test_with_ubuntu_16
        test_on_x86 with: 'ubuntu_16.04'
      end
    
      def test_with_ubuntu_18
        test_on_x86 with: 'ubuntu_18.04'
      end
    
      def test_with_ubuntu_20
        test_on_x86 with: 'ubuntu_20.04'
      end
    
      def test_with_ubuntu_22
       test_on_x86_and_arm with: 'ubuntu_22.04'
      end
    
      def test_with_ubuntu_24
        test_on_x86_and_arm with: 'ubuntu_24.04'
      end
    
      def test_with_archlinux
        test_on_x86 with: 'archlinux'
      end
    
      def test_rockylinux_8
        test_on_x86 with: 'rockylinux_8'
      end
    
      def test_almalinux_8
        test_on_x86 with: 'almalinux_8'
      end
    
      def test_with_macos
        assert_equal('wkhtmltopdf 0.12.6 (with patched qt)', `bin/wkhtmltopdf --version`.strip) if macos?
      end
    
      private
    
      def test_on_x86(with:)
        test_on_docker(with: with) if !macos? && !arm?
      end
    
      def test_on_x86_and_arm(with:)
        test_on_docker(with: with) unless macos?
      end
    
      def test_on_docker(with:)
        assert_match(/wkhtmltopdf 0\.12\.6(.1)? \(with patched qt\)/, `docker-compose run --rm #{with}`.strip)
      end
    end
    CI Configuration

    The new CI workflow includes tests for x86 and ARM architectures. Ensure that all relevant test scenarios are covered, including edge cases for different OS versions and architectures.

    name: CI
    
    on: [push, pull_request]
    
    jobs:
      tests-on-x86-mac:
        runs-on: macos-13
        strategy:
          matrix:
            ruby-version: ['2.3', '2.4', '2.5', '2.6', '2.7', '3.0', '3.1', '3.2']
        steps:
          - uses: actions/checkout@v4
    
          - name: Install Ruby ${{ matrix.ruby-version }} on ${{ matrix.os }}
            uses: ruby/setup-ruby@v1
            with:
              ruby-version: ${{ matrix.ruby-version }}
    
          - name: Install dependencies
            run: bundle install
    
          - name: Run tests with Ruby ${{ matrix.ruby-version }} on ${{ matrix.os }}
            run: bundle exec rake
      tests-on-x86-docker:
        runs-on: ubuntu-latest
        strategy:
          matrix:
            ruby-version: ['3.2']
        steps:
          - uses: actions/checkout@v4
    
          - name: Install Ruby ${{ matrix.ruby-version }} on ubuntu
            uses: ruby/setup-ruby@v1
            with:
              ruby-version: ${{ matrix.ruby-version }}
    
          - name: Install dependencies
            run: bundle install
    
          - name: Run tests on docker(x86)
            run: bundle exec rake
      tests-on-arm:
        runs-on: ubuntu-latest
        strategy:
          matrix:
            ruby-version: ['3.2']
        steps:
          - name: Set up QEMU
            uses: docker/setup-qemu-action@v3
            with:
              platforms: arm64
          - uses: actions/checkout@v4
          - name: Install Ruby ${{ matrix.ruby-version }} on ubuntu
            uses: ruby/setup-ruby@v1
            with:
              ruby-version: ${{ matrix.ruby-version }}
          - name: Run tests on docker(arm64)
            env:
              ARM: 1
              COMPOSE_FILE: docker-compose-arm.yml
            run: |
              set -xeu
              sudo apt update
              sudo apt-get install -y docker-compose          
              bundle install          
              bundle exec rake
    
    

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.