-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Composer phar releases? #318
Comments
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:
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. |
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. |
@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.
Appreciated and I'm sure the community will appreciate it too 😎 |
Because it's less files and the 9P filesystem as used by WSL2 doesn't like many small files performance-wise. I also wanted to learn a bit more GitHub Actions to see if such thing would be possible, and it turns out it is. Maybe one day PHPCS will see an official phar release, see PHPCSStandards/PHP_CodeSniffer#318 for some more info. This required an update of my coding standard to omit setting `installed_paths` config in the ruleset file. Also, the dealerdirect/phpcodesniffer-composer-installer plugin is removed because the custom phar distribution doesn't work with it now, reasons [mentioned in the README](https://github.com/spaze/phpcs-phar/blob/main/README.md). I had to install the slevomat/coding-standard manually and update the config file to use an absolute path, as [documented](https://github.com/spaze/phpcs-phar#modify-the-config-file).
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 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 spaze/phpcs-phar#3 |
@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. |
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 Again, just sharing how I've resolved the problem I had :-) |
Scratching your own itch is not a bad thing. Fair enough, but that means this issue, for now, will remain blocked. |
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. |
As per the issue in the plugin repo (PHPCSStandards/composer-installer#156), I think the paths should be absolute. The composer plugin works locally on install/update/remove, so no portability of the (or be like me and have paths on your laptop and desktop match up, full portability ;-) ) |
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. |
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:
phpcbf
wrapper, thephpcs
wrapper is just arequire
The phar is downloaded automatically,
composer.json
is auto-overwritten too (the phar releasereplace
s just the one regular release).There may be one possibly controversial thing, and that is that the phar release
replace
s 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.
The text was updated successfully, but these errors were encountered: