-
Notifications
You must be signed in to change notification settings - Fork 311
Phar Building #264
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
Phar Building #264
Conversation
| ], | ||
| "autoload": { | ||
| "files": ["autoload.php"] | ||
| "classmap": ["src"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paragonie-scott: Does this change look good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I'm unsure of is the change to using classmap in composer.json. Do we even need to keep autoload.php anymore? If we're keeping autoload.php so that people can require it from a git clone of the repository, then I want composer to use that just so we'll be notified earlier in case it breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm going to revert this one change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, we want to get rid of random_compat, so I'll delete that and autoload.php and keep this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it was all needed, the cleanups were listed as todo items in #258, but somewhy it was closed pointing here, but no back reference to the previous task. i was waiting for base grounds to be settled before actual removing outdated artifacts.
in short: autoload.php file is no longer needed, but change from "files" to "classmap" is needed.
fa83874 to
ecc7f3e
Compare
|
Need to add @paragonie-scott to the authors list in |
|
@paragonie-scott: I added you, please check if I got the right URL and email address :) |
|
Yep, that one works. :) |
| $(call which,$1) | ||
| endef | ||
| box := $(shell which box) | ||
| composer := "composer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems you broke support for box.phar and box executables (same for other tools). most people have .phar extension when they download stuff, but i also have distro package for composer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I added corresponding instructions to the developer docs that they have to be called composer and box in your path. I prefer that over adding complexity to the Makefile, since I'm the only one who should be running it anyway.
|
btw, does the |
|
@glensc: No, it just means I did a rebase of this branch and did a force push (I won't be removing the big images). |
dist/box.json
Outdated
| { | ||
| "in": "src", | ||
| "name": "*.php", | ||
| "exclude": "random_compat" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed now.
|
also from #258 tasklist, remained unresolved: i've heard complaints of having compressed phar being slow you should maybe benchmark this, inspect does the slowness comes due php engine extracting contents to temp dir or just cpu cycles wasted from gzuncompress. also i personally would like to know how .phar and opcache engines (xcache, zendoptimizer, apc) work with phar vs plain filesystem objects. |
Dissenting opinion: That's a lot of work digging into PHP's internals, which might block the release of this library, which could be easily solved by "turn compression off; the deliverable is under 1 MB anyway". |
|
Yeah, let's just turn compression off. |
|
Also TODO: Get travis-CI to build the .phar |
|
Everything's done, just need to understand what those last lines in |
|
Compactors strip the sourcecode before inserting to extract the phar and see: |
|
Thanks, okay this LGTM! |
|
also you can remove |
|
(I tried removing the compactors since it's possible it not being implemented correctly could add errors to the code, but it doesn't seem to work, the result was the same whether those lines were there or they weren't). |
|
the compactors come from composer source i'm pretty sure of that |
|
Ok, after these tests pass I'll merge it. Thanks! |
|
compactor is token based, that's the right way to do it |
|
The compactors won't affect reproducibility verification with Pharaoh, right? |
|
compactor just strips the comments. if you want to diff comments, then it's yeah affected. otherwise executable code stays the same. also, composer.phar has always been stripped like this. |
|
It looks like it messes with the whitespace too (which is where I'm afraid subtle bugs in its compacting algorithm could break things). |
|
But yeah hopefully it's deterministic and shouldn't affect Pharaoh. |
|
of course it's deterministic, there's no side effects of tokenizing static files. |
|
Since it's not compressed, I can probably re-audit the .phar (e.g. with a hex editor) if we have any concerns about backdoored Phar builders. |
|
even if it was compressed, you can decompress it |
Supersedes #258.
WARNING: I force pushed