Skip to content
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.

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

ci: Add GitHub Actions workflow #21

Closed
wants to merge 5 commits into from
Closed

ci: Add GitHub Actions workflow #21

wants to merge 5 commits into from

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jan 9, 2021

Depends on #20.

This will run tests on all supported PHP version 5.6 through 8.0 and check the coding style.

I have decided to use Nix to install different PHP versions since it is a tool I am most familiar with and allows using the same environment locally as on CI. (Think Docker but much more flexible.)

Nix provides nix-shell command which will enter a shell with all the dependencies installed into environment. And if you use it with direnv, you will be put into the environment automatically whenever you cd into the project directory in a terminal emulator.

You can also switch the used PHP version by changing matrix.phpPackage value in the flake.nix file and re-running nix-shell (or touch shell.nix if you use direnv).

For more information, check out fossar/selfoss@e0a0ee2/docs/content/docs/development/using-nix.md

@smottt
Copy link
Owner

smottt commented Jan 10, 2021

I like this, will check once we get the rest sorted out. I have a local set up of some docker containers for multiple PHP versions which also include Xdebug etc and I can configure PHPStorm accordingly. Is it possible to have interpreter & debugger configured correctly with this?

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 10, 2021

You can enable the xdebug extension as follows:

--- a/flake.nix
+++ b/flake.nix
@@ -26,7 +26,9 @@
         pkgs = nixpkgs.legacyPackages.${system};
 
         # Create a PHP package from the selected PHP package.
-        php = phps.packages.${system}.${matrix.phpPackage};
+        php = phps.packages.${system}.${matrix.phpPackage}.withExtensions ({ enabled, all }: with all; enabled ++ [
+          xdebug
+        ]);
       in {
         # Expose shell environment for development.
         devShell.${system} = pkgs.mkShell {

PHP in the shell then has the extension loaded:

$ php -m | grep xdebug
xdebug

Not sure what else needs to be done to set it up for PHPStorm.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 10, 2021

@smottt I cherry-picked changes from #19 and #20 and now the tests pass everywhere:

https://github.com/jtojnar/WideImage/actions/runs/475491846

Had to do some extra changes since PHPUnit 9 does not like assertInternalType (removed) or @expectedException annotation (not sure why).

@smottt
Copy link
Owner

smottt commented Jan 10, 2021

@expectedException has been deprecated years ago, I guess it was also removed at some point.

I think I'll modify the formula for BMP size based on your comment (so it's easier to add support for other than 24-bit if we want that at some point in the future) and merge #19, then #20 and let's see what's left here afterwards.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 10, 2021

Not sure if adding more bit depths is worth it GD supports BMP since PHP 7.2 and BMP is quite obscure format these days.

@smottt
Copy link
Owner

smottt commented Jan 10, 2021

I forgot about that. Seems that TGA is also supported from 7.4 on, so could introduce native support for those two without custom mappers then at some point. Thanks for the tip!

- name: Set up Nix cache
uses: cachix/cachix-action@v8
with:
name: fossar
Copy link
Owner

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way Nix works, it takes build instructions for packages and builds them. It can also substitute the build result from a cache to reduce work. But since these are custom packages, they are not built by the official build servers and we need to use our own cache. I added a comment with a link to the repository that builds it.

composer.json Outdated Show resolved Hide resolved
@@ -0,0 +1,78 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Usually lock files are not committed for libraries. Not sure if this should be here or not, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is lockfile for the CI, not the library. I find it more reliable to have CI state locked and it will also allow you to use the exact same version of dependencies locally in nix-shell.

This will allow people to easily enter a shell with dependencies like PHP and composer pre-installed.

Just install *Nix package manager* and run `nix-shell`.
You can also get *direnv* to be placed in the environment automatically
whenever you `cd` into the project directory in a terminal emulator.

You can also switch the used PHP version by changing `matrix.phpPackage` value
in the `flake.nix` file and re-running `nix-shell`.

For more information, check out https://github.com/fossar/selfoss/blob/e0a0ee2a88fc1be8563a3eb361c7b1142b239232/docs/content/docs/development/using-nix.md
This will allow running tests using `composer test` instead of the hard to remember `sh test/run.sh`.
This will run tests on all supported PHP version 5.6 through 8.0 and check the coding style.
This tool works with PHP 5.6+ so there should hopefully be no issues installing it through composer.
@jtojnar jtojnar closed this Feb 21, 2023
@jtojnar jtojnar deleted the ci branch February 21, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants