Use Box 3 to build the PHAR #3726
Conversation
|
first thing first. before even look at the diff already, please let me treat this PR not as
While I know "good old Box" is not maintained as I would like What i'm wondering about is fact that "3 is being developed" |
Entirely fair :)
Fair question. Maybe I'll need to add a link in the readme about this... A non exhaustive list:
An in general make the tool a whole lot easier to use, less config required, smarter defaults... A more exhaustive (or at least I tried!) list is in the releases notes (yes there is a lot...) There is definitely BC breaks, I try to keep them documented in the upgrade guide. Most of it is I hope not a big deal and under-used features. There is certainly a few cool stuff that I removed, like adding a file to the PHAR via a command, the main reason being because it requires a lot of work, was heavily under tested and I need to stay pragmatic and I think there was more important stuff to focus on |
vendor-bin/box/composer.json
Outdated
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "require": { | |||
| "humbug/box": "^3.0@dev", | |||
There was a problem hiding this comment.
dev means it's fine with any update, and that any update may bring BC breaks, as it's not yet stable.
why not 3.0.0-alpha.4 ?
There was a problem hiding this comment.
Because alpha.3 was released just yesterday :) It's true but that's also why I committed the .lock: you use a fixed version that works and you can try to update at any given time. Since it's alpha, there is potential BC breaks (I try to keep them minimal obviously) so there is not much difference in practice
There was a problem hiding this comment.
you try to convince me I can use symfony/symfony: * instead of symfony/symfony: ^4@stable because i have .lock file ?
well... i'm not convinced.
I want to be able to run composer upgrade for getting non-BC breaks always. having * or @dev on not-yet-stable package will make that impossible.
There was a problem hiding this comment.
Yes you could do it. I'm not suggesting to apply my comment to everything neither: as you said you want to have stable and non-BC breaks so using the right constraint for that helps.
However in the case of Box it is unstable, @dev or @alpha, so it's just a matter of how you manage that instability. Since you try really hard to not rely on a .lock file, would fixing it at a specific version work better here instead of a ranged constraint?
There was a problem hiding this comment.
If using that approach, i'm cutting off possibility to run composer upgrade and receive no BC breaks from performing such action. Not nice for me.
This case here is exception - because you proposed to use not-yet-stable package.
Ultimatelly, we would just put ^3 (= ^3.0@stable)
As I believe, in .json one shall have as wide range of supported versions as possible (, and if he needs .lock file, then locking down to concrete SHA).
Here, it would be concrete alpha, until stable version would be released.
vendor-bin/box/composer.json
Outdated
| { | ||
| "require": { | ||
| "humbug/box": "^3.0@dev", | ||
| "humbug/php-scoper": "^1.0@dev" |
There was a problem hiding this comment.
if we are not scoping, it's not needed, right?
There was a problem hiding this comment.
It's a Box dependency, unfortunately as PHP-Scoper is not in a stable version so just requiring Box will fail saying no stable dependency could be found for PHP-Scoper.
Alternatively I can just add minimal-stability: dev with prefer-stable: true to remove it from the composer.json
There was a problem hiding this comment.
if using single unstable package is not convincing me, then allowing for using any won't convince me either ;)
There was a problem hiding this comment.
maybe php-scoper shall be optional for box in that case ?
There was a problem hiding this comment.
both dependencies are unstable and there, it's just a matter of how you want to display & control them (and in your case you don't care about PHP-Scoper).
Be it PHP-Scoper or some other dependencies which are actually not needed until you use a specific feature of Box, I don't see any easy way to do it.
There was a problem hiding this comment.
super simple example how we did it with optional extension (but it can be done the same with optional library): https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.11/src/Report/XmlReporter.php#L37
it's matters especially with extension, we don't want to force all users to have the extension, if just some of them will use it.
There was a problem hiding this comment.
but yeah, will look definitely better when it become stable with no need to do hacks like that.
|
Hi @theofidry ! Really thanks for detailed answer ;)
Please confirm, it is NOT enabled by default, right ?
exactly, that's why I was looking for some changes from BC point of view. How can I missed UPGRADE guide.... :( While I see value of whole work put there, I don't agree with all things from it, eg
I see value of having single-file distributed dev tools, like online db managers. Looking at the history of fork, there is super big amount of work put into it, while almost no discussion about direction, solution and so. I'm a bit afraid of switching from one single-men project (which become unmaintained over time) into another single-men project :( For above and below, also cc @julienfalque , @SpacePossum , @localheinz One silly question, why General info - if it gonna be applied, it shall target 2.2 LTS, not any 2.x. Going back to initial post...
We were recently discussing internally about some solution for us for tasks, basically walking around makefile, composer tasks and grumphp (keradus@cc5df1f). Basically, with makefile we stucked with cross-platform issues, like we would not be able to execute makefile on Windows env, either as dev, either as CI (vide AppVeyor), which would mean that depends on OS, we have different (or non) tasks runner. Would would be your solution of running makefile on Win? As we definitelly don't want to have multiple tasks runners, and dropping non-Linux OSes is not an option.
Please don't introduce multiple, difference changes as single PR.
It should not. I do agree that deps shall be locked for production applications, even maybe for some devs applications.
Well, same thing here. Instead of assuming we do poorly, just ask. |
No it's a built-in integration but opt-out. You need to register the PHP-Scoper compactor to enable it.
Maybe I need to adjust the readme :)
So regarding the web PHARs: entirely fair. The work however to support them is enormous. There is a lot more parameters to account for and the approach for a web PHAR is relatively different: you don't approach some things the same way: you shouldn't have the shebang, you should not compress the PHAR, there is a lot of parameters to account for... It's not the first thing I removed, but I removed it at the end because it's 1. not a so common use case anymore, 2. something I never ever use/used so hard for me to test and check that I'm doing correctly and 3. I have little time so if the need and help is not there, I prefer to not dig into it :) So if someone is willing to help out to add support for it, I'm fine, otherwise I just cannot afford to.
True. I renamed it actually before removing the web PHAR support, however note that:
History: it's a real fork and I still have hopes to merge it back to the real box project. Kevin was not willing to give me write/admin access to the box orga when I started (which is fair) but I hope he is willing to change his mind now that some work has been done for Box 3.
Arg true. I admit for most of my work I don't have to worry about it since I don't have a windows machine and little people interested to help out with Windows support... Maybe CMake would help? Otherwise I'm sure there is alternatives... I'm sorry I can't help out much there :/
Ah missed that one, will do
The phrasing is off my bad, I did not mean to offend. As long as you're aware of it and have enough e2e tests for it it's all good. |
Edit since I missed that bit. I'm afraid I can't do much to lessen your fears, I would just point that the risk is low. For the sake of transparency, if you look at alternatives (you have an exhaustive list here), they are all abandoned or single-man project... So you either accept that, keep a locked version that work at a given time or roll your own. Also, I'm trying, only time will tell if I will succeed, to get other people in PHAR-related projects on board. But first thing first, a lot of changes in that PR are unnecessary and were here to also test the waters/get quick feedback. I'll finish that PR and make the tests pass (since you already have e2e tests that helps a lot). Then you'll be free to decide if you're willing to take the risk to merge it at this stage, wait a couple of month more for the project to stabilise or if you just prefer to stay on the locked unmaintained box2 version. |
Sadly, nope. too big incompatibilities. Even Symfony dropped idea to officially switch to makefile, exactly because of same reasons... :( |
That was exactly my suggestion behind the scene. If one maintainer is off for few months (like lottery won, new kid in family and so on, so on...) it's just safier to have other maintainer around ;) |
|
@keradus I think I got it ready. I however need to tag a new release on Box to benefit from a bugfix. I'm however a bit confused about one Also I notice that the distributed PHAR is not compressed, is this done on purpose to avoid requiring the Now to go back to the original question of what changes Box 3 brings, here are some numbers when comparing the PHAR built with Box 2 and Box 3. Note that the numbers may change with the official new Box release (although I doubt by much). Box 2 (2.7.5): Box 3: A non exhaustive list of differences of benefits brought by Box 3:
You can see the exact diff here and notice that Box 3 actually manages to add less files. If there is more files in the end this is due to the requirement checker feature (which can be disabled if you want to). |
You forgot about requets of mine:
not sure what bugfix you refer to...
As explicitly stated in
indeed.
we already have shebang in our entry file...
IMO, would be way more readable if written in clasic octal form of
Sth went wrong. Our current size using Box 2 is 1.61MB
That check is performed when one is running box3, or when one is running phar built by box3? |
not sure do I read that diff correctly, but definitely we don't want to have those files in our phar release:
|
|
Thanks for the quick answer!
I found a bug that I patched locally. I need to submit a proper bug fix, tag it and use the tag release here for completing this PR.
There is a change there that I didn't notice. I didn't realise your main script would be used as the stub in Box 2 when no stub setting was configured and with In Box 3, unless using the PHAR default stub ( #!/usr/bin/env php
<?php
/*
* Generated by Humbug Box.
*
* @link https://github.com/humbug/box
*/
Phar::mapPhar('box-auto-generated-alias-5ae8a99539e74.phar');
require 'phar://box-auto-generated-alias-5ae8a99539e74.phar/.box/check_requirements.php';
require 'phar://box-auto-generated-alias-5ae8a99539e74.phar/php-cs-fixer';
__HALT_COMPILER(); ?>This is done to allow the PHAR to be renamed into something else more easily and integrate the requirement checker. You also can have a specific alias if you set the So the shebang line is added to that stub, and the one in your main script is removed. Note that Box2 was doing something similar if you were using the Box2 stub ( I think this choice is simpler down the line as it allows you to worry about less stuff which can lead to some simplifications in the main script (which I admit I didn't look at before so I can do so and see what can be removed/changed). If you do prefer to have complete control then we can keep things at it is, i.e. use the main script as a stub. In this case however we'll need to add a hardcoded box internal stuff to include the requirement checker unless you want to ditch that feature.
Hm indeed looks like By removing all the extra files manually (17 of them) + the requirement checker file, I get The fresh build is gives
Note that enabling the JSON compactor (also existing in Box2) shrinks it to Those files are here because they are required for Box to be able to dump the autoloader with the classmap authoritative mode. However this feature is originally there because required for PHP-Scoper and because most often than note the PHARs are compressed hence the small file diff is negligible. If however this is a deal breaker for you I'll complete box-project/box#188. Some of the
So again here: you're call. Note however that the second choice requires box-project/box#188 since otherwise dumping the autoloader will fail. Last but not least the biggest change in the PHAR size here comes from the requirement checker which is of |
|
Sorry that it's another super long answer... |
well, for that you also need to wait for my answer ;)
We DO have have
So, I believe we shall stack into original stub file of ours.
Please, I would like to have our stab with our logic. Then... I took a deeper look into requirements checker of Box3.
once more, they ARE part of composer classmap and they are available for end user if he install PHP CS Fixer as his dependency for legacy reasons,
Or, maybe simplest solution would be to not follow classmap authoritative mode for PHP CS Fixer Box3's config, but hardcoded finder, like we had for Box2?
as already stated, box3-built-in requirement checker shall not be part of |
|
well, I believe that you were not expecting to have such troubles with this PR ;) |
I wonder how I missed that... ok should solve the issue to keep the PR minimal
I'll think about it if there is enough demand for it. For now since Box3 may be merged back to the original box project and since I already got ~20 packages to manage, if I can avoid one more package to manage I'll do so :P
Hmm you're right. Originally those classes were genuinely from Symfony Requirements, I removed the dependency later on since the implementation required was simpler and slightly different. I completely missed that in the process for some classes. I opened #192 to not forget to add it back.
Let's keep it at that then. At least now you know once this legacy part can be dropped the config can be simplified :)
It actually doesn't care: Composer just require the existence of certain files for the dump autoload (
I missed that somehow, but fair enough it can easily be disabled so I'll do so. As to the size: yes. The reality is I broke my teeth on it for a good 3 weeks or so before coming with an acceptable implementation. It's a bit tricky since it requires:
You can however see that the source code is relatively simple so what is expensive here is the pure no compression + Composer autoloading. So to answer your question: there may be some optimisations that can be done, notably by being able to ditch the Composer for the autoloading and instead concatenate several classes into one file. Right now however for the sake of simplicity, I leveraged the PHAR feature and PHP-Scoper instead which I hope is "good enough" for most situations as a first iteration although I do agree the price in size might be off-putting when no compression is used.
I was expecting some resistance & finding some bugs, although certainly not that much :) I admit I'm a bit surprised by how energy draining the process is, but well, that's how it's bound to be I guess so no blaming here since I would probably behave the same in your stead. For the sake of transparency, I'll probably have a 1-2 weeks off since I have some personal stuff to attend to and a few newly risen issues to fix in Box. So don't be worried if you don't get notified on something before then on this PR! |
still not sure where 28 MB are. I saw only 280 KB
well, at least there are tests for things that got broken and failing Travis :D
men wanted to be nice, discussed everything in detail, explain, not just refuse the idea. and... boom ;P |
280KB yes... sorry too tired |
|
Hi @theofidry , |
|
@theofidry , Sorry for not being as easy going as SpacePossum ;) |
5b63be1 to
4be32e6
Compare
14c48e6 to
59b7255
Compare
|
Done |
|
@theofidry just changing merge target isn't enough, you need to actually rebase, because now the PR includes all the 2.13 commits |
|
@dmvdbrugge yes this is what I've done, the PR no longer include the 2.13 changes. It does includes the commit so but that shouldn't be an issue if the PR is squashed; @keradus is that alright or I should squash it myself? |
|
Huh, your changes reflect that indeed, however the commits still show up. Must be some github weirdness, sorry for the noise. |
|
I care that diff is valid, I squash during merging. |
59b7255 to
a42955e
Compare
|
Done |
|
@keradus are you waiting on anything from my side or you just need more time to review it? Just to know if there is anything I can do to push it forward or if I should just patiently wait :) |
|
just to be sure: it doesn't need to be me to review it ;) answering your question - sorry, lately i was totally out of dev due to some things in private life, I need to catch up with github now, this PR is one of 240 on my list :( |
|
np, just wanted to double check :) |
|
Wether it's lack of time or lack of interest, it's been 8 months since the last update. I just have no interest to invest more energy in this so I'll be closing this. |
|
Hi @theofidry . First of all, big sorry for not having time to deal with this PR. I feel ashamed because of that. Unfortunately, each of us have plenty of things to do and not enough time to do them all. Here, I finally manage to done with my checks and it looks all nice now indeed! I also played with few updates on box side, but i'm afraid we can't benefit from it until 3.0, where we done some cleanup of project (eg fuckup we have on v2 about autoloading and crazy workaround what we expose in git-export or phar). |
|
Awesome work on preparing Box 3, enhancing it during PHP CS Fixer update to Box 3 and the update on PHP CS Fixer itself! Great debugging and nice findings to make the switch possible, @theofidry. Big kudos ! |
This PR was merged into the 2.12 branch. Discussion ---------- Use Box 3 to build the PHAR This PR is a suggestion to switch from the unmaintained Box 2 to the Humbug fork in which the version 3 is being developed. A few things worth mentioning: - I used a `Makefile` instead of a bash file as I think it's an improvement for this kind of task, there is a better discoverability (type `make` and you could get the list of the commands) and it's easier to manage the task dependencies, i.e. get that `make smth` and it just works experience, no need to figure out how to install Box, do a `composer install` or something. - I used [`bamarni/composer-bin-plugin`](https://github.com/bamarni/composer-bin-plugin) to install Box as a dependencies without polluting the library dependencies. I can switch that to a PHAR if you prefer, but in any case I think it's better to have a way which does not require contributors to figure out which tool they need to install and better not require them a tool globally on their machine. If you are interested in more on the topic, I wrote an [entire article](https://medium.com/@tfidry/managing-your-dependencies-in-php-321d584441ab) about it I also noticed the `composer.lock` is not committed. IMO it should: it sure does not make any difference to the user when consuming the project as a library, but when you are shipping a PHAR you are shipping a real application on which it's better to have control on what dependencies is used there. On the same token, I didn't find any e2e tests for the PHAR. I would _highly_ recommend to have one. Updating all the deps, adding some on the fly and shipping all the stuff in the PHAR and wish that it just works is being a bit insouciant. The PHAR transformation is not a trivial process although Box try very hard to make it easier, there is still code that just don't work within the PHAR. I've pushed that against 2.x as it's still the default branch, but it can be easily ported to 3.x as well Commits ------- b25d20b Update Box 3 integration 86f03cd Upgrade to Box 3
|
Great! Thanks for the heads up. FYI there is a |
This PR is a suggestion to switch from the unmaintained Box 2 to the Humbug fork in which the version 3 is being developed.
A few things worth mentioning:
Makefileinstead of a bash file as I think it's an improvement for this kind of task, there is a better discoverability (typemakeand you could get the list of the commands) and it's easier to manage the task dependencies, i.e. get thatmake smthand it just works experience, no need to figure out how to install Box, do acomposer installor something.bamarni/composer-bin-pluginto install Box as a dependencies without polluting the library dependencies. I can switch that to a PHAR if you prefer, but in any case I think it's better to have a way which does not require contributors to figure out which tool they need to install and better not require them a tool globally on their machine. If you are interested in more on the topic, I wrote an entire article about itI also noticed the
composer.lockis not committed. IMO it should: it sure does not make any difference to the user when consuming the project as a library, but when you are shipping a PHAR you are shipping a real application on which it's better to have control on what dependencies is used there.On the same token, I didn't find any e2e tests for the PHAR. I would highly recommend to have one. Updating all the deps, adding some on the fly and shipping all the stuff in the PHAR and wish that it just works is being a bit insouciant. The PHAR transformation is not a trivial process although Box try very hard to make it easier, there is still code that just don't work within the PHAR.
I've pushed that against 2.x as it's still the default branch, but it can be easily ported to 3.x as well