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

boesing · 2022-08-10T18:01:21Z

Q	A
Bugfix	yes
BC Break	no
New Feature	yes

Description

By using composers own composer/semver library, we are able to detect PHP version ranges from almost every composer-compatible constraint.

Fixes #87

…version ranges Signed-off-by: Maximilian Bösing <[email protected]>

Signed-off-by: Maximilian Bösing <[email protected]>

boesing · 2022-08-10T19:07:04Z

Dockerfile

@@ -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


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:

ensure that renovate is able to bump versions

provide alpine PHP version within this container

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...

~~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

package.json

src/config/app.ts

internalsystemerror · 2022-08-11T15:26:04Z

package.json

@@ -5,9 +5,9 @@
  },
  "type": "commonjs",
  "devDependencies": {
+    "@types/node": "^18.6.4",


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

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

internalsystemerror · 2022-08-11T15:29:09Z

Dockerfile

@@ -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


~~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

src/config/app.ts

internalsystemerror · 2022-08-11T15:33:46Z

src/semver.ts

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


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

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

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:

^v8.1.0 would be a valid constraint

Comma annotations are valid constraints (as outlined in Composer php constraints with , are not supported #86)

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.

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).

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?

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?

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.

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.

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?

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

internalsystemerror · 2022-08-11T15:44:04Z

Everything checks out though. Looks good to merge if we decide to go down this route, but I'm not convinced its necessary.

internalsystemerror · 2022-08-11T19:28:15Z

Dockerfile

+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


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

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

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...

Ocramius · 2022-08-11T20:08:35Z

Dockerfile

+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


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

Ocramius · 2022-08-11T20:10:47Z

tests/issue-86/.laminas-ci.json

@@ -0,0 +1,11 @@
+{


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

boesing · 2022-08-11T20:37:56Z

I've raised an "issue" in the NPM semver implementation:
DefinitelyTyped/DefinitelyTyped#61669

internalsystemerror · 2022-08-11T20:53:07Z

As I understand it, that repository is for the typescript mappings for everything @types/ related, including @types/semver. The implementation for the semver library would be https://github.com/npm/node-semver. I've opened an issue for that one here npm/node-semver#468. 🤞

internalsystemerror · 2022-08-12T18:27:26Z

It appears there are some other differences, so I think we have to go this route after all.

boesing added the Enhancement label Aug 10, 2022

boesing force-pushed the feature/composer-semver branch from bee5e98 to 15c66c4 Compare August 10, 2022 18:10

feature: introduce semver via composer/semver to better detect PHP …

de55ea6

…version ranges Signed-off-by: Maximilian Bösing <[email protected]>

boesing force-pushed the feature/composer-semver branch from 15c66c4 to de55ea6 Compare August 10, 2022 18:12

qa: add test case for laminas#86

56a8d2e

Signed-off-by: Maximilian Bösing <[email protected]>

boesing linked an issue Aug 10, 2022 that may be closed by this pull request

Composer php constraints with , are not supported #86

Open

bugfix: job within matrix needs to be a string

49ae7dc

Signed-off-by: Maximilian Bösing <[email protected]>

boesing requested a review from Ocramius August 10, 2022 19:05

boesing commented Aug 10, 2022

View reviewed changes

package.json Show resolved Hide resolved

boesing commented Aug 10, 2022

View reviewed changes

src/config/app.ts Show resolved Hide resolved

internalsystemerror reviewed Aug 11, 2022

View reviewed changes

boesing mentioned this pull request Aug 11, 2022

Implement dependency injection #120

Open

internalsystemerror reviewed Aug 11, 2022

View reviewed changes

Ocramius reviewed Aug 11, 2022

View reviewed changes

boesing marked this pull request as draft August 11, 2022 22:15

Ocramius changed the base branch from 1.14.x to 1.15.x August 18, 2022 11:52

internalsystemerror mentioned this pull request Sep 13, 2022

Add in composer-require-checker as a globally installed tool #131

Open

boesing changed the base branch from 1.15.x to 1.23.x December 23, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

boesing commented Aug 10, 2022

boesing Aug 10, 2022

internalsystemerror Aug 11, 2022

internalsystemerror Aug 11, 2022

boesing Aug 11, 2022

internalsystemerror Aug 11, 2022

internalsystemerror Aug 11, 2022 •

edited

Loading

internalsystemerror Aug 11, 2022

boesing Aug 11, 2022

internalsystemerror Aug 11, 2022

boesing Aug 11, 2022

internalsystemerror Aug 11, 2022

weierophinney Aug 11, 2022

boesing Aug 11, 2022

boesing Aug 11, 2022

internalsystemerror Aug 11, 2022

internalsystemerror commented Aug 11, 2022

internalsystemerror Aug 11, 2022

Ocramius Aug 11, 2022

boesing Aug 11, 2022

Ocramius Aug 11, 2022

Ocramius Aug 11, 2022

boesing commented Aug 11, 2022

internalsystemerror commented Aug 11, 2022

internalsystemerror commented Aug 12, 2022

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

Are you sure you want to change the base?

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

Conversation

boesing commented Aug 10, 2022

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internalsystemerror Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internalsystemerror commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boesing commented Aug 11, 2022

internalsystemerror commented Aug 11, 2022

internalsystemerror commented Aug 12, 2022

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

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

internalsystemerror Aug 11, 2022 •

edited

Loading