PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
889 stars 54 forks source link

Composer phar releases? #318

Open spaze opened 7 months ago

spaze commented 7 months ago

Sometimes, I prefer the phar distribution of various tools, and I know PHPCS creates phars both as release assets and downloadable, and I know there's Phive too, but I still like to manage the versions with Composer.

To mostly scratch my own itch, I have created a phar repository of PHPCS hosted here https://github.com/spaze/phpcs-phar I also wanted to learn GitHub Actions a bit more, to see if it's possible to do what I intended to do.

I have seen some other PHPCS phar Composer packages (for example), but they seemed to be updated manually, which I wanted to avoid at all cost.

So I've built it completely automated: if PHP_CodeSniffer releases a new version, GitHub Actions workflow will pick it up (in 2 hours latest, can be configured), commit a new version, check if it works, verify the signature and create a new release with the same version number.

You can see the 3.8.1 phar release here https://github.com/spaze/phpcs-phar/releases/tag/3.8.1 I don't need older releases so I'm not planning on back-adding them.

As I said, I've created this to be used by me mostly, but I was wondering whether you'd like to bring it under the PHPCSStandards GitHub organization? It may complement the regular package, similar to vimeo/psalm and psalm/phar packages, it may be useful for some other folks, but may as well not be useful to anyone else, either way I don't really mind :-)

I could help maintain this repo of course, as I'd maintain my repo anyway, but I don't expect much issues or PRs in this particular case.

Things I'd expect to be maintained from time to time, but probably very rarely:

The phar is downloaded automatically, composer.json is auto-overwritten too (the phar release replaces just the one regular release).

There may be one possibly controversial thing, and that is that the phar release replaces the "dealerdirect/phpcodesniffer-composer-installer" plugin. The reason is that it doesn't work with the phar release package anyway as documented so there's no need to install it.

The advantage of doing it this way, instead of publishing phars to the phar repo from the build action for example, is that it would require zero work in the PHP_CodeSniffer repository, no new tokens, no nothing, unless you'd like the phar Composer package to be created immediately. Personally I don't mind, it's an acceptable compromise for me, and the time-to-release could changed from 2 hours to whatever you'd like.

There's really not much to license (and my repo doesn't contain one currently) but happy to use and/or add BSD-3 licence to follow PHPCS.

Please let me know if it sounds good, and even if it doesn't. Thanks.

jrfnl commented 7 months ago

Hi @spaze, thank you for opening this issue.

The primary reason why packages like Psalm and phpDocumentor release PHAR-based Composer packages is that they have a lot of runtime dependencies, which often cause conflicts with the runtime/dev dependencies of projects using these tools.

That is not the case for PHP_CodeSniffer, which, at this time, does not have any runtime dependencies, though that may change in the future.

I'd already decided that if/when that happens, a PHAR-based Composer package would be needed (and should possibly become the primary), so I'm open to the idea you are proposing.

However, there are some caveats:

  1. Disabling the Composer installer package is not acceptable. This will lead to extra support requests for maintainers of external standards, while they have explicitly included the Composer installer package to prevent those support requests. Yes, this will mean that changes are needed in the Composer installer package, but that's fine. And making those changes would be a pre-requisite. There is already an issue open in the repo about using absolute paths instead of relative ones, for instance.
  2. Yes, the package would need to be licensed under BSD-3 - and should already be licensed as such, as the code in the package being distributed is licensed as such. You are probably already in violation of the license as things are now by not included it.
  3. Like done for this repo, all releases since the 2.0.0 release would need to be tagged and made available.
  4. As the package currently doesn't have dependents (yet) (or very few), I would prefer a package rename under the phpcsstandards organisation.

Obviously the workflow and anything else in the repo would need to be reviewed and validated before a repo move as well, but that's a given.

spaze commented 7 months ago

Hi, thanks. My personal motivation for a phar distro is the suboptimal performance of the 9P filesystem as used by WSL2.

I understand there are some blockers, mainly your points 1. & 3. For 2 I have added a license file now (and will plan to do an hour or two of community service for violating the BSD-3 license, sorry for that. Can send pics 😎). 4. sure, that would go hand in hand with the repo move. I have added a warning to the README that the package will cease to exist one day.

jrfnl commented 7 months ago

@spaze 1 is the real blocker. 3 we can sort out with the repo move and probably use some scripting for, along the lines of what I did for #205. Don't worry about that one for now.

For 2 I have added a license file now (and will plan to do an hour or two of community service for violating the BSD-3 license, sorry for that. Can send pics 😎).

Appreciated and I'm sure the community will appreciate it too 😎

spaze commented 2 months ago

Btw, I've solved the standards installer package support, or rather lack of, by not requiring the plugin at all and replacing it with Composer + PHPCS API usage, scanning installed packages and installing available standards on each phpcs execution.

Composer has a nice API for that:

InstalledVersions::getInstalledPackagesByType('phpcodesniffer-standard');

and PHPCS as well:

Config::setConfigData('installed_paths', implode(',', $paths), true);

The composer API can be required with

    "require": {
        "composer-runtime-api": "^2.1"
    },

I don't know whether this would make up for the current plugin installer package "incompatibility", I think functionally it does, at least for me, but for completeness, here's the PR in my repo https://github.com/spaze/phpcs-phar/pull/3

jrfnl commented 2 months ago

@spaze Thanks for working on this, however, I think you're trying to re-invent the wheel instead of fixing what's already there. The chosen path is very inefficient and will slow PHPCS down, while we've been trying to improve performance. There's a reason why the Composer plugin actually hooks into Composer and not into PHPCS.

spaze commented 2 months ago

Oh, absolutely reinventing the wheel! :-)

I'm mostly scratching my own itch here, and just sharing how I'm scratching it. Not really trying to fix anything, sorry. Tried a different approach, because relative paths don't work in phars, but absolute paths wouldn't work for me either. So this seemed like the only possible way - using relative paths but before the code in the phar is executed.

I believe the installer plugin hooks, or hooked, into Composer because the Composer API I'm using here is available since 2021 and didn't exist back when the plugin was first created, which is why the plugin was created in the first place. I may be wrong though.

However, I don't agree it's very inefficient but we may agree to disagree. The auto-install code runs for 3ms on my 4yo laptop running in WSL. I haven't compared it to using the .php config file but that's something I can live with. (To put the number in perspective, my codebase is analysed in ~250ms, when results are cached, 8 sec when not.)

Again, just sharing how I've resolved the problem I had :-)

jrfnl commented 2 months ago

Scratching your own itch is not a bad thing. Fair enough, but that means this issue, for now, will remain blocked.

spaze commented 2 months ago

Agree, it is still blocked :-)

Thinking about it, I don't think the problem can be solved with creating the config file, which is what the installer plugin does, either with relative paths (they're relative to the files inside the phar then), or absolute paths (because they're absolute and can't be for example shared between systems - why would anyone do that I don't know, maybe because they can?). That's why the wheel was reinvented because honestly, at this point, I don't even know what needs to be fixed.

jrfnl commented 2 months ago

As per the issue in the plugin repo (https://github.com/PHPCSStandards/composer-installer/issues/156), I think the paths should be absolute.

The composer plugin works locally on install/update/remove, so no portability of the conf file is required, as the plugin is portable and all that's needed to "fix" the conf file would be to (re-)run composer install on whatever machine you want to run PHPCS on.

(or be like me and have paths on your laptop and desktop match up, full portability ;-) )

spaze commented 2 months ago

The issue also mentions flags and some VMs and what not, so it seems it's not easy to create a "simple fix" which I totally understand as changing something and covering all cases is difficult. Besides, neither way would work for me personally.

I don't have a desktop but I have a CI server I cannot really set up the paths the same way on. And I'm not running composer on it. This may not be a common setup (neither is running PHPCS as phar probably), but I think it should still be supported, and it is (which I'm happy honestly it works)!

As you pointed out, you can "fix" the paths by running composer again, but now I can just run PHPCS where it takes 3ms longer to auto-config it. I could overengineer it with docker running on my machine and the CI server, and prod, and... and one day I probably will because I always like to overengineer stuff, sometimes! 😅

Thanks for the "chat" anyway.