GH Action: Add steps to test against libxml >= 2.12 #798
Conversation
asispts
force-pushed
the
bugfix/issue-767-action
branch
from
ee93a6a to
bcc81fc
Compare
|
@jrfnl done |
There was a problem hiding this comment.
Thank you for working on this @asispts ! I really appreciate your help in stabilizing this part of the test suite and safeguarding the functionality in different circumstances.
I do have some questions and remarks, which I've left in line. Questions do not necessarily imply something needs to change. In most cases, I'd just like to understand the reasoning why.
| # Don't cancel in-progress matrix jobs | ||
| fail-fast: false |
There was a problem hiding this comment.
The omission of fail-fast is deliberate. Please remove this change as it is not necessary and unrelated to addressing the issue at hand.
| @@ -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) }} | |||
There was a problem hiding this comment.
I have a number of question about this:
- 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)
- How was it decided to test against PHP 8.1, 8.2 and 8.3, but not against PHP 8.4 or 8.5 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -93,6 +96,20 @@ jobs: | |||
| - name: Checkout code | |||
| uses: actions/checkout@v4 | |||
|
|
|||
| # A temporary solution | |||
There was a problem hiding this comment.
How and why is this a temporary solution ? When could the temporary solution be removed ? Why was it added ?
I don't mean to sound overly critical, but in my opinion, documentation should provide information and this documentation raises questions without answering them.
Please replace this with a short note explaining the why of the step, at what point it could be removed and what would be the criteria which would need to be met for removal.
| 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 |
There was a problem hiding this comment.
This contains a hard-coded version - 2.12.9 - and I doubt dependabot would be able to update it.
Is there a 2.12-latest reference which could be used which would always download (possibly via an HTTP redirect) the latest version of libxml 2.12 ?
If not, how can we make sure the workflow stays up to date ?
On that note: libxml 2.13 is already out. Should we also have a build testing against libxml 2.13 ?
|
Compiling libxml2 on each build sounds expensive. Would it be faster to use Homebrew for the installation? Also, do we need to test with every PHP version? Wouldn't it be enough if we installed the custom libxml2 only on let's say PHP 8.4 and use Ubuntu's bundled libxml2 for the other builds? |
Description
Follow up of #797.
I've update GH action to run tests against
libxml >= 2.12. Also, I addedfail-fast: falseto ensure that other matrix jobs continue running until completion.Note
Since
libxml >= 2.12is not available inubuntu-latest, the library is compiled from source.Suggested changelog entry
Related issues/external references
Types of changes
PR checklist