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

PHP: Check lowest versions of dependencies in CI #2035

Merged
merged 8 commits into from
Jul 4, 2022

Conversation

ciaranmcnulty
Copy link
Contributor

@ciaranmcnulty ciaranmcnulty commented Jul 4, 2022

This is mostly because of #2034 - we need to check that everything still works with the 'lowest' versions of the dependencies that are allowed by composer.json

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with older versions of Messages any more, or when we've written tests that don't work with older versions of the test tools

This is expected to fail until #2034 is merged

@aurelien-reeves
Copy link
Contributor

Do you confirm that as long as #2034 is not merged, the job gherkin-php-bc should fail?

@ciaranmcnulty
Copy link
Contributor Author

ok I need help with make :/

I have this in the Makefile:

composer update ${COMPOSER_FLAGS:-}

the new jobs are running:

composer update

@aurelien-reeves
Copy link
Contributor

aurelien-reeves commented Jul 4, 2022

(I copy/paste my answer on slack)

It looks like using variables in a makefile is not the same as with bash

refs. https://www.gnu.org/software/make/manual/html_node/Using-Variables.html

So, two possibilities:

using ifdef as we do in gherkin/go/Makefile (or its counterpart ifndef like in gherkin/javascript/Makefile)

or using $(VAR) instead of ${VAR} but here I don't know how to process unset variables

@aurelien-reeves
Copy link
Contributor

Actually you should be able the braces too (${}). I think here what is not working is the :- that is a bash feature and seems not working with make

@ciaranmcnulty
Copy link
Contributor Author

Nice work @aurelien-reeves

@ciaranmcnulty
Copy link
Contributor Author

ciaranmcnulty commented Jul 4, 2022

Do you confirm that as long as #2034 is not merged, the job gherkin-php-bc should fail?

Yes. I suggest we merge that one (it looks fine to me) and then rebase this PR.

It's good that we have 'seen it fail'

@ciaranmcnulty
Copy link
Contributor Author

Grargh

Ok this doesn't work because we point to the 'local' messages with some config in composer.json, so --prefer-lowest hasn't pulled in the older package

It's probably still worth merging for the other minor fixes but I'll try and see a clean way to remove the config (in just that job)

@ciaranmcnulty
Copy link
Contributor Author

ok it's 'successfully' failing now

ciaranmcnulty and others added 8 commits July 4, 2022 13:06
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
This is used in static analysis and older versions can't deal with enums directly

Becuase we don't depend on it directly Psalm was allowing an older version to install
This is something we still want to check for even if the types say it's redundant - JSON decoding could be
throwing an error here
@aurelien-reeves
Copy link
Contributor

I guess this should now pass!?

Once everything is ready, let me know for a last review :)

@ciaranmcnulty
Copy link
Contributor Author

Yes I think it can merge if passed (it failed on gherkin-php-bc just before rebase)

@aurelien-reeves aurelien-reeves enabled auto-merge (squash) July 4, 2022 12:19
@@ -466,6 +479,21 @@ jobs:
name: gherkin/php
command: |
cd gherkin/php
composer config --unset repo.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some comments here to explain why we need to do this?

@aurelien-reeves aurelien-reeves merged commit bacd05a into main Jul 4, 2022
@aurelien-reeves aurelien-reeves deleted the add-php-lowest-jobs branch July 4, 2022 12:48
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.

2 participants