Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSwitch to Box #488
Merged
Switch to Box #488
Conversation
|
The current PR doesn't build a functioning PHAR I got it to work by extracting the bit of the current Compiler that builds the PHAR stub, then using that stub for Box (plus a few other misc changes): diff --git a/.editorconfig b/.editorconfig
index 779f99a..fddf9c1 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -10,3 +10,6 @@ insert_final_newline = true
[*.md]
trim_trailing_whitespace = false
+
+[Makefile]
+indent_style = tab
diff --git a/.gitignore b/.gitignore
index 823cceb..221aa8a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,3 +10,4 @@ __pycache__
psysh.phar
psysh-compat.phar
/vendor-bin/*/vendor/
+build/stub
diff --git a/Makefile b/Makefile
index c70876a..c9741ad 100644
--- a/Makefile
+++ b/Makefile
@@ -26,6 +26,9 @@ build: bin/psysh.phar
# Rules from files
#---------------------------------------------------------------------------
+build/stub: bin/build-stub bin/psysh LICENSE
+ bin/build-stub
+
composer.lock: composer.json
composer install
@@ -38,5 +41,5 @@ vendor/bamarni: composer.lock
vendor-bin/box/vendor: vendor/bamarni
composer bin box install
-bin/psysh.phar: bin/psysh src vendor box.json.dist vendor-bin/box/vendor
+bin/psysh.phar: bin/psysh src vendor box.json.dist vendor-bin/box/vendor build/stub
vendor/bin/box compile
diff --git a/bin/build-stub b/bin/build-stub
new file mode 100755
index 0000000..0d26110
--- /dev/null
+++ b/bin/build-stub
@@ -0,0 +1,22 @@
+#!/usr/bin/env php
+<?php
+
+$license = file_get_contents(dirname(__DIR__) . '/LICENSE');
+$license = str_replace('The MIT License (MIT)', '', $license);
+$license = str_replace("\n", "\n * ", trim($license));
+
+$autoload = <<<'EOS'
+ Phar::mapPhar('psysh.phar');
+ require 'phar://psysh.phar/.box/check_requirements.php';
+ require 'phar://psysh.phar/vendor/autoload.php';
+EOS;
+
+$content = file_get_contents(dirname(__DIR__) . '/bin/psysh');
+$content = preg_replace('{/\* <<<.*?>>> \*/}sm', $autoload, $content);
+$content = preg_replace('/\\(c\\) .*?with this source code./sm', $license, $content);
+
+$content .= '__HALT_COMPILER();';
+
+@mkdir(dirname(__DIR__) . '/build');
+
+file_put_contents(dirname(__DIR__) . '/build/stub', $content);
diff --git a/box.json.dist b/box.json.dist
index 1351364..c25684f 100644
--- a/box.json.dist
+++ b/box.json.dist
@@ -1,3 +1,10 @@
{
- "chmod": "0755"
-}
\ No newline at end of file
+ "stub": "build/stub",
+ "chmod": "0755",
+ "blacklist": [
+ "Test"
+ ],
+ "compactors": [
+ "KevinGH\\Box\\Compactor\\Php"
+ ]
+}
diff --git a/composer.json b/composer.json
index 909d069..3e169be 100644
--- a/composer.json
+++ b/composer.json
@@ -22,7 +22,8 @@
},
"require-dev": {
"phpunit/phpunit": "~4.8.35|~5.0|~6.0|~7.0",
- "hoa/console": "~2.15|~3.16"
+ "hoa/console": "~2.15|~3.16",
+ "bamarni/composer-bin-plugin": "^1.2"
},
"suggest": {
"ext-pcntl": "Enabling the PCNTL extension makes PsySH a lot happier :)",
The The
Travis already handles the deployment https://github.com/bobthecow/psysh/blob/master/.travis.yml#L28-L38
I'm curious how you're getting 649KB PHARs. Even after enabling the PHP compactor and blacklisting "Test", to make it a bit more of a fair comparison, I'm only down to 1.7MB.
PsySH (both the See a4f181d |
Makefile
Outdated
| clean: ## Clean all created artifacts | ||
| .PHONY: clean | ||
| clean: | ||
| git clean --exclude=.idea/ -ffdx |
theofidry
Apr 26, 2018
Author
Contributor
Just don't do it when you have uncommitted changes :P If you have a way to check that so we could bail out at that time that would be
Note that it has never been a big deal for me because even when I did the mistake PHPStorm has always been able to restore everything without an issue
bobthecow
Apr 26, 2018
Owner
Not only does this blow away uncommitted changes (without asking), but it does it while ignoring local and global git excludes. So even if your git status doesn't show anything dirty, it will delete things you explicitly excluded.
We should be explicit about what we make dirty, and explicit about cleaning them. This is why the current build always builds everything into build/ and installs composer dependencies needed for build in build-vendor/. Then we can blow those two things away, and ignore everything else.
theofidry
Apr 26, 2018
Author
Contributor
but it does it while ignoring local and global git excludes
Hm I forgot about that part, I probably didn't mind it because the only thing not committed I never needed was that .idea... But I see how this can be an issue. Maybe we can change that to a softer one.
We should be explicit about what we make dirty, and explicit about cleaning them. This is why the current build always builds everything into build/ and installs composer dependencies needed for build in build-vendor/. Then we can blow those two things away, and ignore everything else.
It's a fair PoV and we can adjust to change it. My usage was slightly different and more extreme: I really wanted to get rid of everything (except IDE config) that I wouldn't get after a git clone. I used it a lot to make sure that git clone project; cd project; make smth just worked without any user step (although I ended up failing on that for Box since it needs docker...)
| vendor: composer.lock | ||
| composer install | ||
|
|
||
| vendor/bamarni: composer.lock |
| rm -rf build-vendor | ||
|
|
||
| COMPOSER_VENDOR_DIR=build-vendor composer update \ | ||
| --prefer-stable --no-dev --no-progress --classmap-authoritative --no-interaction --verbose |
bobthecow
Apr 26, 2018
Owner
This does a few things that aren't captured in the current makefile:
- uses a separate composer root, to ensure that the vendor directory is clean (and to build without screwing with the user's current vendor directory)
- prefers stable, doesn't install dev dependencies
- uses an optimized autoloader and classmap-authoritative to put more of the burden of autoloading on build time rather than runtime
theofidry
Apr 26, 2018
•
Author
Contributor
I'm not sure to follow 1, but 2 & 3 are done by Box to avoid too much faff to the users
bobthecow
Apr 26, 2018
Owner
1 is what makes it so we can run composer update without messing with the user's versions or even worrying about them at all. It uses build-vendor/ to install a fresh set of dependencies, as up-to-date as dependency hell will allow :)
theofidry
May 5, 2018
Author
Contributor
Updated the makefile for it to never change whatever you currently have in composer.json, composer.lock or vendor
bin/package
Outdated
| @@ -1,55 +0,0 @@ | |||
| #!/usr/bin/env bash | |||
bobthecow
Apr 26, 2018
Owner
This is the part that builds the package for releases. We shouldn't delete it until we have a replacement.
theofidry
Apr 26, 2018
•
Author
Contributor
You mean it builds all the PHARs right? Or you mean it packages them in tar.gz? If the second I think this should be done in the Makefile instead
bobthecow
Apr 26, 2018
Owner
I mean this is what builds the tarballs. We need this for releases. Makefile would be a great place for it.
Makefile
Outdated
| #--------------------------------------------------------------------------- | ||
|
|
||
| composer.lock: composer.json | ||
| composer install |
bobthecow
Apr 26, 2018
Owner
if the lock file is outdated, this doesn't work. it needs to run composer update.
theofidry
Apr 26, 2018
Author
Contributor
I would eventually add an @echo but I wouldn't do composer update, updating your deps (for an app, not a lib) should be done more careful reviewing which deps needs to be updated, what changed and see that everything is compatible. Unless you have enough e2e tests to test the PHAR and being able to blindly update everything in absolute confidence
|
Yes I noticed it but I was really confused at the autoloading part in the first place... I'll give you the diff later of the files shipped in the PHAR (you can see it with So you need that custom stub :/ It's a shame it would be nicer not to have to bug I guess it can't be helped. |
|
Update: Build on master`psysh.phar` details`psysh-compat.phar` detailsBuild with BoxBox `psysh.phar` detailsBox `psysh-compat.phar` details |
| .php_cs.cache | ||
| psysh.phar | ||
| psysh-compat.phar | ||
| /build/ |
theofidry
May 5, 2018
Author
Contributor
I added the / prefix, it's usually a better practice to ensure those rules applies only to this project.
| clean: ## Clean all created artifacts | ||
| .PHONY: clean | ||
| clean: | ||
| rm -rf build/ |
theofidry
May 5, 2018
Author
Contributor
so I updated the script, there is only two artefact directories ever created: dist and build. Everything is in there
| vendor: composer.lock | ||
| composer install | ||
|
|
||
| vendor/bamarni: composer.lock |
|
|
||
| build: ## Compile the application into the PHAR | ||
| .PHONY: build | ||
| build: build/psysh.phar build/psysh-compat.phar build/psysh-php54.phar build/psysh-php54-compat.phar |
theofidry
May 5, 2018
Author
Contributor
So the PHP 5.4 PHARs are being built thanks to the Composer platform.php option which ensures the deps are installed as everything was on 5.4. So the 4 PHARs are generated here instead of 2 PHARs in two separate builds.
Note that the non PHP 5.4 PHAR is currently built in 7.1, you may want to lower that
bobthecow
May 5, 2018
Owner
I think I tried building on 7.2, 7.1 and 7.0, and the one that worked most consistently across PHP versions was 7.0. Let's keep that, for now, and possibly bump it later.
| @@ -0,0 +1,22 @@ | |||
| #!/usr/bin/env php | |||
theofidry
May 5, 2018
Author
Contributor
I opened an issue box-project/box#206 to be able to remove the main script since your stub ditches it. It's however a minor optimization that shouldn't be blocking
| rm -rf build-vendor | ||
|
|
||
| COMPOSER_VENDOR_DIR=build-vendor composer update \ | ||
| --prefer-stable --no-dev --no-progress --classmap-authoritative --no-interaction --verbose |
theofidry
May 5, 2018
Author
Contributor
Updated the makefile for it to never change whatever you currently have in composer.json, composer.lock or vendor
| tar --transform "s/.*/psysh/" -czf dist/psysh-${PKG_VERSION}.tar.gz psysh.phar | ||
| tar --transform "s/.*/psysh/" -czf dist/psysh-${PKG_VERSION}-compat.tar.gz psysh-compat.phar | ||
| fi | ||
| tar -czf dist/psysh-${PKG_VERSION}.tar.gz build/psysh.phar |
bobthecow
May 5, 2018
Owner
The reason for the previous code was to rename all the phars on the way in to just be psysh. Unfortunately the options to do this aren’t portable, hence the “if BSD” business.
|
@bobthecow ready for another round |
Makefile
Outdated
| rm -rf build/psysh || true | ||
| mkdir build/psysh | ||
| cp -R $(PSYSH_SRC) build/psysh/ | ||
| composer config --working-dir build/psysh-php54-compat platform.php 7.1 |
Makefile
Outdated
| rm -rf build/psysh-compat || true | ||
| mkdir build/psysh-compat | ||
| cp -R $(PSYSH_SRC) build/psysh-compat/ | ||
| composer config --working-dir build/psysh-php54-compat platform.php 7.1 |
|
From a fresh install, the makefile won't work (because |
bin/package
Outdated
| fi | ||
| tar -czf dist/psysh-${PKG_VERSION}.tar.gz build/psysh.phar | ||
| tar -czf dist/psysh-compat-${PKG_VERSION}.tar.gz build/psysh-compat.phar | ||
| tar -czf dist/psysh-php54-${PKG_VERSION}.tar.gz build/psysh-php54.phar |
bobthecow
May 5, 2018
Owner
we can leave off the php version check higher in the package script, since we're packing all at once.
bin/package
Outdated
| tar -czf dist/psysh-${PKG_VERSION}.tar.gz build/psysh.phar | ||
| tar -czf dist/psysh-compat-${PKG_VERSION}.tar.gz build/psysh-compat.phar | ||
| tar -czf dist/psysh-php54-${PKG_VERSION}.tar.gz build/psysh-php54.phar | ||
| tar -czf dist/psysh-php54-compat-${PKG_VERSION}.tar.gz build/psysh-php54-compat.phar |
| @@ -22,8 +22,7 @@ | |||
| }, | |||
| "require-dev": { | |||
| "phpunit/phpunit": "~4.8.35|~5.0|~6.0|~7.0", | |||
| "symfony/finder": "~2.1|~3.0|~4.0", | |||
| "hoa/console": "~2.15|~3.16" | |||
theofidry
May 6, 2018
Author
Contributor
Hm, I think I messed up an update at one point, sorry didn't see I removed it
Makefile
Outdated
| @@ -0,0 +1,88 @@ | |||
| .DEFAULT_GOAL := help | |||
|
|
|||
| PSYSH_SRC = bin src box.json.dist composer.json composer.lock build/stub | |||
bobthecow
May 5, 2018
Owner
let's not copy the composer.lock file over. the current installed state shouldn't have any impact on the built phars.
|
The |
|
I'm still seeing ~1.7MB box-build phars, at a minimum. How did you get 649KB? |
|
Ha! It looks like ¯\_(ツ)_/¯ Removing |
|
Ok. After digging through dependencies of all the dependencies, the Every other extension we might want can't be (or hasn't been) polyfilled. So let's add |
I see the same, they were just compressed at one point which you probably don't want unless you don't mind the PHAR not being usable because the user does not have the |
|
Addressed most comments if not all. Note that Box requirement checker takes 280KB with ~30 files. You can also have a great diff thanks to |
|
I also think the stub could be reworked a bit to run the requirement checker as the very very first thing when in the PHAR, before the autoloading |
|
I made a suggestion with a different stub which would remove the need for the |
|
The requirement checks shouldn't run if we're just using the stub as an autoloader. The thing I really like about the current stub is that it doesn't require anything to run. Not even We definitely wouldn't want requirement checks to run in that case, since they're about compatibility with the guts of the phar, not about compatibility with the code that's actually going to run :) |
Got it. Yep, we don't want a compressed phar.
Cool. I'm not trying to nitpick on size, I'm mostly using it as a gauge for whether we're including a bunch of stuff we don't actually need.
Yep. That's what I've been using to track things down :) |
This time it's for a different reason :) https://github.com/bobthecow/psysh/blob/master/bin/psysh#L99-L100 |
|
Let's keep the |
|
Checked all four variations, and the contents of the phars looks right. Sizes look right too:
That's + ~300k for the box requirements checker, and + ~ 1.2MB to each of the compat builds for the new polyfills. |
What I don't like about the current stub generation is:
But let's do that then, it can always be improved later.
Sounds right. The deployment in Travis need to be updated still though |
|
Thanks so much for taking this on! I'll open a pull request shortly for you to review with a few more ideas. Side note: I missed it this time, but please open pull requests against |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
As promised somewhere, here's the PR to switch to Box to build the PHAR.
I used a
Makefilehere because I would like to propose it as a new tool instead of the bash files. Indeed in think it has a few advantages:make smthand it will just work experience, all the necessary stuff will be installed for you.As an example, clone the project and type
make buildand it will just work: no need to concern yourself with installing the dependencies & co.There is still stuff to improve in this PR (or can be done in another):
For reference the PHAR you are publishing is 1.4MB (from the last release). The one built with Box is😄
649.59KBso it's a nice improvementI'm also not sure to understand why we are using the local autoload when inside the PsySH project?