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.

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

Jump to bottom

Feature: Use composer/semver to detect PHP version ranges #119

Draft
wants to merge 3 commits into
base: 1.23.x
Choose a base branch
from
Draft

Feature: Use composer/semver to detect PHP version ranges #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de55ea6
feature: introduce semver via `composer/semver` to better detect PHP …
boesing Aug 10, 2022
56a8d2e
qa: add test case for #86
boesing Aug 10, 2022
49ae7dc
bugfix: job within matrix needs to be a string
boesing Aug 10, 2022
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
14 changes: 13 additions & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ RUN npm ci
COPY ./src ./src
RUN npm run build


FROM ghcr.io/boesing/composer-semver:1.0 as composer-semver
FROM php:8.1.9-cli-alpine as php
FROM node:18-alpine

LABEL "repository"="http://github.com/laminas/laminas-ci-matrix-action"
LABEL "homepage"="http://github.com/laminas/laminas-ci-matrix-action"
LABEL "maintainer"="https://github.com/laminas/technical-steering-committee/"
Expand All @@ -25,6 +27,16 @@ ADD laminas-ci.schema.json /action/
COPY --from=compiler /usr/local/source/dist/main.js /action/
RUN chmod u+x /action/main.js

# Setup PHP
Copy link
Member Author

Choose a reason for hiding this comment

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

I know, what the fuck. But since I am not aware on how to actually merge multiple container together, I've thought I can do it like this.

This will:

  1. ensure that renovate is able to bump versions
  2. provide alpine PHP version within this container
  3. allows us to run PHP natively within the matrix container while the container itself is optimized for nodejs

I hope we can keep it like this as I tried to install php8-cli via apk which is PHP 8.1.1 which is not supported by the "boxed" PHAR of my composer-semver docker image...

Copy link
Member

Choose a reason for hiding this comment

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

Does the matrix container run any PHP? What happens with PHP 8.0 or PHP 7.4 runs? nm, I see where it's used now

RUN mkdir -p /usr/local/bin /usr/local/etc /usr/local/lib
COPY --from=php /usr/local/bin/php /usr/local/bin
COPY --from=php /usr/local/etc/* /usr/local/etc
COPY --from=php /usr/local/lib/php/* /usr/local/lib/php
COPY --from=php /usr/lib/* /usr/lib
COPY --from=php /lib/* /lib
Copy link
Member

Choose a reason for hiding this comment

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

it's these parts that worry me mixing the languages. if they both rely on a linked lib in different versions for instance.

Copy link
Member

Choose a reason for hiding this comment

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

Similar thoughts: would prefer a composer install happening with our base image, but we can improve on that later

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need for composer install, thats actually to execute the composer-semver.phar I am building with https://github.com/boesing/composer-semver-docker

But the more I think about this, the more it annoys me that it has to be that hacky...


COPY --from=composer-semver /usr/local/bin/main.phar /usr/local/bin/composer-semver.phar

ADD entrypoint.sh /usr/local/bin/entrypoint.sh

ENTRYPOINT ["entrypoint.sh"]
33 changes: 19 additions & 14 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
},
"type": "commonjs",
"devDependencies": {
"@types/node": "^18.6.4",
Copy link
Member

Choose a reason for hiding this comment

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

did you move this or a tool? Only I've seen tools move it the other way around before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this was due to a merge conflict. So maybe I did that.

"@typescript-eslint/eslint-plugin": "^5.16.0",
"@typescript-eslint/parser": "^5.16.0",
"@types/node": "^18.6.4",
"eslint": "^8.11.0",
"eslint-config-incredible": "^2.4.2",
"eslint-import-resolver-typescript": "^2.7.0",
Expand All @@ -25,7 +25,6 @@
"dependencies": {
"@actions/core": "^1.6.0",
"@cfworker/json-schema": "^1.12.2",
"@types/semver": "^7.3.9",
"semver": "^7.3.5"
"child_process": "^1.0.2"
boesing marked this conversation as resolved.
Show resolved Hide resolved
}
}
3 changes: 2 additions & 1 deletion src/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ export class App {
this.checkRequirements(filesFromDiff),
this.composerJsonFileName,
this.composerLockJsonFileName,
this.continousIntegrationConfigurationJsonFileName
this.continousIntegrationConfigurationJsonFileName,
this.logger
);
}

Expand Down
25 changes: 20 additions & 5 deletions src/config/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import fs, {PathLike} from 'fs';
import semver from 'semver';
import parseJsonFile from '../json';
import {Tool, ToolExecutionType} from '../tools';
import {Logger} from '../logging';
import {satisfies} from '../semver';
import {CURRENT_STABLE, INSTALLABLE_VERSIONS, InstallablePhpVersionType, isInstallableVersion} from './php';
import {ComposerJson} from './composer';
import {ConfigurationFromFile, isAdditionalChecksConfiguration, isAnyComposerDependencySet, isAnyPhpVersionType, isConfigurationContainingJobExclusions, isExplicitChecksConfiguration, isLatestPhpVersionType, isLowestPhpVersionType, JobDefinitionFromFile, JobFromFile, JobToExcludeFromFile} from './input';
Expand All @@ -16,19 +16,33 @@ export enum ComposerDependencySet {
LATEST = 'latest',
}

function gatherVersions(composerJson: ComposerJson): InstallablePhpVersionType[] {
function gatherVersions(composerJson: ComposerJson, logger: Logger): InstallablePhpVersionType[] {
boesing marked this conversation as resolved.
Show resolved Hide resolved
if (JSON.stringify(composerJson) === '{}') {
logger.debug('The composer.json file is either empty or does not exist.');

return [];
}

const composerPhpVersion: string = (composerJson.require?.php ?? '').replace(/,\s/, ' ');

if (composerPhpVersion === '') {
logger.debug('`composer.json` does not contain any PHP requirement.');

return [];
}

return INSTALLABLE_VERSIONS
.filter((version) => semver.satisfies(`${version}.0`, composerPhpVersion));
.filter((version) => {
if (satisfies(`${version}.0`, composerPhpVersion)) {
logger.debug(`PHP ${version} is supported by projects \`composer.json\`!`);

return true;
}

logger.debug(`PHP ${version} is NOT supported by projects \`composer.json\`!`);

return false;
});
}

function gatherExtensions(composerJson: ComposerJson): Set<string> {
Expand Down Expand Up @@ -418,12 +432,13 @@ export default function createConfig(
requirements: Requirements,
composerJsonFileName: PathLike,
composerLockJsonFileName: PathLike,
continousIntegrationConfigurationJsonFileName: PathLike
continousIntegrationConfigurationJsonFileName: PathLike,
logger: Logger
): Config {
const composerJson: ComposerJson = parseJsonFile(composerJsonFileName) as ComposerJson;
const configurationFromFile: ConfigurationFromFile =
parseJsonFile(continousIntegrationConfigurationJsonFileName) as ConfigurationFromFile;
const phpVersionsSupportedByProject: InstallablePhpVersionType[] = gatherVersions(composerJson);
const phpVersionsSupportedByProject: InstallablePhpVersionType[] = gatherVersions(composerJson, logger);
let phpExtensions: Set<string> = gatherExtensions(composerJson);
let stablePHPVersion: InstallablePhpVersionType = CURRENT_STABLE;

Expand Down
11 changes: 11 additions & 0 deletions src/semver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import {execSync} from 'child_process';

export function satisfies(version: string, constraint: string): boolean {
try {
execSync(`/usr/local/bin/composer-semver.phar semver:match "${version}" "${constraint}" > /dev/null`);
Copy link
Member

@internalsystemerror internalsystemerror Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There are some edge case differences between how composer and npm resolve semantic versions (or so I'm led to believe), but I don't see why we aren't using the official semver module used by npm https://www.npmjs.com/package/semver and not even require PHP here

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I know this PR is removing it and replacing it with composer, I'm just wondering why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we do parse composer.json, that file could provide constraints which are mostly supported by composer but i.e. not by the NPM package.

I have not tested this yet, but composer provides some quirks:

There might be other quirks but since I do not want to hassle with these in this component since we should focus on detecting the correct supported version(s) based on what is supported by composer, we should use composers own logic to detect whats allowed and what not.
But thats just my opinion here.

Copy link
Member

Choose a reason for hiding this comment

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

That first one would be a valid constraint in node aswell FYI. I did look through the docs for both npm and composer and the comma vs space was the only one I found. Which IMO is npm's mistake here as I know of other examples where a comma is perfectly valid (e.g. go).

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no strong opinion on this, I just would prefer not to manipulate the constraint to "make it work" 😅
But I am open for every other suggestion. Should we report this upstream?

Copy link
Member

Choose a reason for hiding this comment

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

what about calling the php container via docker here instead? I'm not that familiar with how DinD works on github specifically but something like:

execShell('docker run php /usr/local/bin/composer-semver.phar semver:match "${version}" "${constraint}" > /dev/null'). After adding composer-semver to the php container instead?

Copy link
Member

Choose a reason for hiding this comment

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

Won't work; you're operating inside a container, so the container can not operate as a docker host.

As far as other suggestions... I don't have any. I used the NPM semver package originally, as I assumed it was compatible with how Composer handled semver. Knowing that they're different, we can either:

  • Call on a PHP executable (as proposed here)
  • Write our own TS utility that passes the same tests as composer/semver (lots more effort, plus it can go out-of-date)

It might make sense to re-do this action such that it uses a dedicated container where we use a PHP application, as we've done with the CI action and the automatic releases action.

Copy link
Member Author

Choose a reason for hiding this comment

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

Won't work; you're operating inside a container, so the container can not operate as a docker host.

Thats actually not true. It definitely works, I am just not sure if it works within GHA.
One has to mount the docker socket to the container tho.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am still wondering if we should just report upstream that they can't handle , in the constraint?
Maybe its a fixable issue for them?

Copy link
Member

Choose a reason for hiding this comment

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

Issue opened npm/node-semver#468, we can see what they say


return true;
} catch {
return false;
}
}
11 changes: 11 additions & 0 deletions tests/issue-86/.laminas-ci.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

instead of issue-86, would it make sense to name this according to what it does?

"checks": [
{
"name": "Whatever Check",
"job": {
"php": "*",
"command": "test"
}
}
]
}
5 changes: 5 additions & 0 deletions tests/issue-86/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"require": {
"php": ">=5.6,<=8.1.99"
}
}
52 changes: 52 additions & 0 deletions tests/issue-86/matrix.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
{
"include": [
{
"name": "Whatever Check [5.6, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"5.6\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [7.0, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"7.0\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [7.1, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"7.1\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [7.2, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"7.2\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [7.3, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"7.3\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [7.4, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"7.4\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [8.0, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"8.0\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
},
{
"name": "Whatever Check [8.1, latest]",
"operatingSystem": "ubuntu-latest",
"action": "laminas/laminas-continuous-integration-action@v1",
"job": "{\"command\":\"test\",\"php\":\"8.1\",\"extensions\":[],\"ini\":[],\"dependencies\":\"latest\",\"ignore_platform_reqs_8\":false,\"ignore_php_platform_requirement\":false,\"additional_composer_arguments\":[]}"
}
]
}