Skip to content

GH Action: Add steps to test against libxml >= 2.12 #798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

GH Action: Add steps to test against libxml >= 2.12 #798

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3310914
Run tests against libxml >= 2.12
asispts Jan 25, 2025
bcc81fc
Fix yaml format
asispts Jan 25, 2025
ceb01eb
Remove fail-fast
asispts Jan 27, 2025
b54082e
Install libxml2 via homebrew
asispts Jan 27, 2025
e25ee62
Separate install libxml2 steps
asispts Jan 27, 2025
be3c27f
Fix yaml
asispts Jan 27, 2025
19bf218
Update steps
asispts Jan 27, 2025
0ce96c6
Use double quote
asispts Jan 27, 2025
ad85bfa
Update comment and fix yaml
asispts Jan 28, 2025
1d715e3
Use pre-installed homebrew
asispts Jan 28, 2025
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
17 changes: 17 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ jobs:
# Keys:
# - custom_ini: Whether to run with specific custom ini settings to hit very specific
# code conditions.

# Don't cancel in-progress matrix jobs
fail-fast: false
matrix:
os: ['ubuntu-latest', 'windows-latest']
php: ['5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3', '8.4', '8.5']
Expand Down Expand Up @@ -93,6 +96,20 @@ jobs:
- name: Checkout code
uses: actions/checkout@v4

# A temporary solution
- name: Install libxml2 >= 2.12 (PHP 8.1+ on linux only)
if: ${{ matrix.os == 'ubuntu-latest' && contains(fromJSON('["8.1", "8.2", "8.3"]'), matrix.php) }}
Copy link
Member

Choose a reason for hiding this comment

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

I have a number of question about this:

  1. Does this have to be tested on multiple PHP versions or would testing on a single PHP 8.1+ version be sufficient ? (the compile from scratch slows the build down with ~45-60 second extra build time)
  2. How was it decided to test against PHP 8.1, 8.2 and 8.3, but not against PHP 8.4 or 8.5 ?

Copy link
Member

Choose a reason for hiding this comment

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

As for the usability/comprehension of failures if/when they would occur:
As things are, if a build using libxml 2.12 would fail, it is not transparent for a non-regular contributor that libxml 2.12 is being used and may be the cause of the failure. They would have to study the workflow to figure that out.

Did you consider making the use of libxml 2.12 a matrix flag ? And if you considered this, what was the reason to decide against that ?
Note: if it would be a matrix flag, that would also allow for annotating that libxml 2.12 is being used for a certain job in the job name.

Copy link
Member

Choose a reason for hiding this comment

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

Last question: not sure if that would work, but did you consider/have you tried installing libxml 2.12 via the extensions key in the setup-php action runner ?
Ref: https://github.com/shivammathur/setup-php?tab=readme-ov-file#heavy_plus_sign-php-extension-support

Copy link
Contributor Author

@asispts asispts Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jrfnl The issue is related to libxml2 library, not the PHP libxml extension.

I've added a comment to explain why is this a temporary solution. Also, I used homebrew, as suggested by @derrabus, to install latest libxml2 version and remove the hardcoded version.

Copy link
Member

@jrfnl jrfnl Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@asispts Thanks for your continued work on this.

While my last question (third comment here) has been answered, the questions posed in my first two comments have not yet been answered and I'd still be interested to understand the reasoning for the current choices.

run: |
sudo apt-get update
sudo apt-get install -y wget build-essential
wget https://download.gnome.org/sources/libxml2/2.12/libxml2-2.12.9.tar.xz
tar -xf libxml2-2.12.9.tar.xz
cd libxml2-2.12.9
./configure --prefix=/usr/local
make
sudo make install
sudo ldconfig

- name: Setup ini config
id: set_ini
shell: bash
Expand Down
Loading