laminas / laminas-ci-matrix-action

GitHub Action for creating a CI job matrix.
BSD 3-Clause "New" or "Revised" License
24 stars 15 forks source link

Missing support for `psalm/phar` #301

Open zonuexe opened 4 months ago

zonuexe commented 4 months ago

Bug Report

Q A
Version(s) 1.39.0

Summary

When Psalm is installed using psalm/phar instead of vimeo/psalm, an error occurs because vendor/vimeo/psalm/config.xsd does not exist.

refs https://github.com/laminas/laminas-ci-matrix-action/pull/31, https://github.com/psalm/phar/issues/5

Current behavior

Running before_script: xmllint --schema vendor/vimeo/psalm/config.xsd psalm.xml
warning: failed to load external entity "vendor/vimeo/psalm/config.xsd"
<?xml version="1.0"?>
Schemas parser error : Failed to locate the main schema resource at 'vendor/vimeo/psalm/config.xsd'.
WXS schema vendor/vimeo/psalm/config.xsd failed to compile
<psalm xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="https://getpsalm.org/schema/config" errorLevel="1" resolveFromConfigFile="true" xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" errorBaseline="psalm-baseline.xml" findUnusedBaselineEntry="true" findUnusedCode="false">
    <projectFiles>
        <directory name="src"/>
        <ignoreFiles>
            <directory name="vendor"/>
        </ignoreFiles>
    </projectFiles>
</psalm>

How to reproduce

In a project where psalm.xml exists, replacing vimeo/psalm with psalm/phar will reproduce the problem.

https://github.com/michaelpetri/phpunit-consecutive-arguments/pull/109

Expected behavior

No errors.

Ocramius commented 4 months ago

Evidently, vendor/vimeo/psalm/config.xsd is an invalid declaration for your project: provide an XSD at any right location, and you should be fine.

zonuexe commented 4 months ago

@Ocramius

Isn't vendor/vimeo/psalm/config.xsd also hardcoded into laminas-continuous-integration-action?

https://github.com/laminas/laminas-ci-matrix-action/blob/v1.25/src/tools.ts

Both Psalm and the application need to be improved to resolve this issue.

boesing commented 4 months ago

It is, that is due to the fact that the CI implementation does only support psalm installed via composer as vimeo/psalm. That was mainly as the generated phar has a bunch of problems, especially because it does not conflict with vimeo/psalm dependency and thus, you can have composer installed vimeo/psalm as v4.20 while the psalm/phar is 5.30.

I already reported that in https://github.com/psalm/phar/issues/14 2 years ago and thus, for now, we do not support custom psalm config path.

Feel free to provide a PR where the XSD path for psalm can be modified but that would require you to have the XSD file somewhere available. I guess that this is not available from within psalm/phar?

Ocramius commented 4 months ago

@zonuexe good shout, sorry for jumping to conclusions!

boesing commented 4 months ago

I moved this issue to the CI matrix action as it has to be implemented here. I also had a quick glance into the psalm/phar package, which sadly does not provide the XSD. So you could ofc. create a copy of that in your project and extend the CI matrix to consume configurable XSD path but that feels odd. Maybe psalm/phar can pack the XSD along with the phar, that would actually ensure the correct XSD file (feel free to create a feature request there).

Until then, to make your CI work, you could add your own additional_checks entry where you omit the config validation stuff. Via exclude you could then drop the psalm check which is auto-generated from the matrix job: https://github.com/laminas/laminas-ci-matrix-action?tab=readme-ov-file#excluding-specific-jobs

I still have https://github.com/laminas/laminas-ci-matrix-action/pull/184 pending, I could extend that to allow excluding tools as well. 🤔

Ocramius commented 4 months ago

@boesing I'm thinking that we should perhaps honor the XSD location, when specified as a local path: WDYT?

boesing commented 4 months ago

@Ocramius you mean extracting from psalm.xml?

Ocramius commented 4 months ago

Or generally any XML: we currently use hardcoded locations, but the locations are in the individual XML diles already, when done right 👍

boesing commented 4 months ago

the hardcoded location is used as that is known from psalm. but I am up for extracting the location from the config file - but usually there is no actual path located in XML files. I see either just namespaces and/or URI targeting external files via https. Even phpunit generated config is targeting remote XSD. I do not really want to implement a whole XSD detection strategy just for a niche use-case. we do not support psalm.xml in any other location than project root as well so I wanted to keep it simple. From what I can see, having a config option would probably work best (config is available where we do create the Tool job) but extracting stuff from XML might open a can of worms. but maybe I think too complicated?

Ocramius commented 4 months ago

do not really want to implement a whole XSD detection strategy just for a niche use-case. we do not support psalm.xml in any other location than project root as well so I wanted to keep it simple.

This is fair 👍

boesing commented 4 months ago

I created https://github.com/psalm/phar/issues/18 to request config.xsd being bundled with upcoming versions. Lets see how that works out. In the meantime, we could continue on working out how to have the XSD location being configured via the .laminas-ci.json.

Maybe something like this?

{
    "...": {},
   "tools": {
      "psalm": {
          "config": "pathToAlternativePsalm.xml",
          "schema": "pathToConfigSchemaDefinition.xsd"
      }
   }
}

We do already have quirks for infection where we detect roave plugin for the executable: https://github.com/laminas/laminas-ci-matrix-action/blob/f25365c8538fd59ba0c3f8bb52e5a22e20f6fc39/src/tools.ts#L40-L46

I would assume that we have something like this in-place for the psalm/phar detection to swap to the bundled config.xsd in that components folder (once its bundled).


Also created a patch in psalm to have the config.xsd bundled. https://github.com/vimeo/psalm/pull/10955

boesing commented 4 months ago

Just to have my 2 cents dropped here regarding the usage of psalm/phar: I wanted to use that a few years back as well, thats why I mentioned https://github.com/psalm/phar/issues/14 in my 1st post. The biggest problem with the PHAR is that u are unable to use psalm plugins at all. IMHO, there are a bunch of decent plugins which provide more in-depth stuff such as the doctrine plugin, etc.

Thats why I ended up keeping vimeo/psalm in our projects, even tho having psalm/phar in-place would help us in reducing dependency conflicts. So depending on what the main goal of the initial change was to switch to psalm/phar, I would at least think about the plugin problematic once more. But ofc, for most stuff, the psalm as is, is already good enough.

zonuexe commented 4 months ago

psalm/phar is important for michaelpetri/phpunit-consecutive-arguments because psalm is not compatible with PHP-Parser 5 and PHPUnit 11 until they release Psalm v6.

refs https://github.com/vimeo/psalm/pull/10567, https://github.com/vimeo/psalm/issues/10788 and https://github.com/vimeo/psalm/pull/10888

boesing commented 4 months ago

I released v1.26.0 for the matrix action, you can - until we found a better solution - use exclude now to exclude auto-generated Psalm jobs.

Something like this is the repository you are mentioning could be implemented as .laminas-ci.json:

{
    "additional_checks": [
        {
             "name": "Static Code Analysis via psalm.phar",
             "job": {
                 "command": "./vendor/bin/psalm"
                 "php": "@lowest",
                 "dependencies": "locked"
             }
        }
    ],
    "exclude": [
       {"name": "Psalm"}
   ]
}

(I think the path is wrong so that would be another thing to consider when providing a patch for the alternative XSD path 😅)


Btw. I've read about your example regarding the extraction of the config.xsd. If that is something we can actually do, I would not mind having that in before_script 🤷🏼‍♂️ If that is how that has to work, could also be used for the standalone example Bruce mentioned (i.e. phive). But I'd say lets start with the psalm/phar example and see if some1 will ever need this to be supported for phive - I mean, we do not support phive installed phpunit so lets keep it simple for now.

boesing commented 4 months ago

Do you think you can provide a PR for that feature @zonuexe? I'd be happy to review and create a new release for that, just hit me up! In case you need help, you can reach out on slack, I have the same name there as on github 👍🏼