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
2 changes: 1 addition & 1 deletion .gitattributes
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/.gitattributes export-ignore
/.github/ export-ignore
/.gitignore export-ignore
/.travis.yml export-ignore
/Doxyfile export-ignore
/phpunit.xml export-ignore
/tests export-ignore
56 changes: 56 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# https://help.github.com/en/categories/automating-your-workflow-with-github-actions

on:
pull_request:
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK

  pull_request:
    types: [opened, synchronize]

should suffice, i.e. we don’t need to re-run the build for other PR actions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can change this.

Out of curiosity: Are there any other actions? (Closing a PR does not trigger a build AFAIK.)

Copy link
Collaborator

@sabberworm sabberworm Apr 19, 2021

Choose a reason for hiding this comment

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

There are the following pull_request activity types:

  • assigned
  • unassigned
  • labeled
  • unlabeled
  • opened
  • edited
  • closed
  • reopened
  • synchronize
  • ready_for_review
  • locked
  • unlocked
  • review_requested
  • review_request_removed

But, what I wasn’t aware of, when you don’t specify types explicitly, only opened, synchronize and reopened trigger builds so we’d only get rid of reopened so I guess you can leave it as-is.

(Though I would assume reopened PRs still have their previous builds lying around so I don’t know if there’s a need to actually rebuild but since reopens are relatively rare, it shouldn’t matter).

push:
schedule:
- cron: '3 3 * * 1'

name: CI

jobs:
unit-tests:
name: Unit tests

runs-on: ubuntu-20.04

strategy:
fail-fast: false
matrix:
php-version:
- 5.3
- 5.4
- 5.5
- 5.6
- 7.0
- 7.1
- 7.2
- 7.3
- 7.4

steps:
- name: Checkout
uses: actions/checkout@v2

- name: Install PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-version }}
tools: composer:v2
coverage: none

- name: Cache dependencies installed with composer
uses: actions/cache@v1
with:
path: ~/.cache/composer
key: php${{ matrix.php-version }}-composer-${{ hashFiles('**/composer.lock') }}
restore-keys: |
php${{ matrix.php-version }}-composer-

- name: Install Composer dependencies
run: |
composer update --with-dependencies --no-progress;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should always install from the lock file for CI builds.

Suggested change
composer update --with-dependencies --no-progress;
composer install --no-progress;

There can be a separate action to update dependencies regularly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I've adapted those two changes. (I have also proposed a PR to get rid of composer.lock as only project should have this file, but not libraries: #209)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I hadn’t yet seen that PR, sorry. Is this recommendation documented anywhere? I would prefer to not break builds just because dependencies have released updated patch versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know whether this is documented somewhere. I learned this from a talk on Composer by Nils Adermann (one of the persons behind Packagist).

The basic idea is:

  • Projects have a composer.lock as they are expected to be deployed with exactly specified versions of their dependencies, and changes to the dependencies should be separate explicit changes.
  • Libraries are expected to work with all allowed versions of their dependencies. So they should have no composer.lock, and CI should run the tests (at least the unit tests) both with the lowest as well as the highest versions of the dependencies. (This is something for which I will provide a PR once this is PR here merged.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this change has broken the build. I guess (without being able to spend time on this before later today) that we need different sets of dependency versions for different PHP versions.

I'll have a look at this later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has turned out that we need to keep using composer update. If we use composer install, the installation will fail if the composer.lock contains package versions that are not installable with the current PHP version: https://github.com/oliverklee/PHP-CSS-Parser/runs/2381731583?check_suite_focus=true

I'll undo this change in a minute, squash and repush.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. You can see the build status here on my fork: https://github.com/oliverklee/PHP-CSS-Parser/actions/runs/764969412

(In your repository, the builds will only start after the merge.)

composer show;

- name: Run Tests
run: ./vendor/bin/phpunit
22 changes: 0 additions & 22 deletions .travis.yml

This file was deleted.

2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PHP CSS Parser
--------------

[![build status](https://api.travis-ci.org/sabberworm/PHP-CSS-Parser.svg)](https://travis-ci.org/sabberworm/PHP-CSS-Parser)
[![Build Status](https://github.com/sabberworm/PHP-CSS-Parser/workflows/CI/badge.svg?branch=master)](https://github.com/sabberworm/PHP-CSS-Parser/actions/)

A Parser for CSS Files written in PHP. Allows extraction of CSS files into a data structure, manipulation of said structure and output as (optimized) CSS.

Expand Down