Skip to content
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.

Sign up for GitHub

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 improvements #389

Closed
wants to merge 10 commits into from
Closed

PHAR improvements #389

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
312e913
phar compiler performance, include vendor, fix autoload, added compos…
mssimi Mar 27, 2018
8648452
revert autoload, enable phar composer
mssimi Mar 28, 2018
c17676e
rm phar to boost performance of re-builds
mssimi Mar 28, 2018
674d7db
remove autoloader check
mssimi Mar 29, 2018
ebf19f6
phar ignore
mssimi Mar 29, 2018
21201ae
remove PharFilesFinder
mssimi Mar 29, 2018
6d94217
move phar to build dir
mssimi Mar 30, 2018
8dd9e4a
added glob resource test
mssimi Mar 30, 2018
d18c970
added rector.phar for glob test
mssimi Mar 30, 2018
cadcd37
fix phpstan
mssimi Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ composer.lock

# phar related
/build
/build-phar
rector.phar
5 changes: 5 additions & 0 deletions .pharignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
build
docs
tests
phpunit
.git
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ script:
# build phar test - 1st group is "build-phar" test step by step; 2nd group is all-together
- |
if [[ $BUILD_PHAR != "" ]]; then
packages/PharBuilder/bin/compile
composer compile-phar
php rector.phar
vendor/bin/phpunit --testsuite=phar-build
fi
Expand Down
9 changes: 8 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,14 @@
"vendor/bin/ecs check bin packages src tests --fix",
"bin/clean_levels.sh"
],
"phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon"
"phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon",
"compile-phar": [
"rm -f rector.phar",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reasons we've talked about and WTF like #319 (comment)
I'd recommend to use nested directory for build, e.g. common build, so file in build/rector.phar would fail on missing vendor/autoload.php.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about bin/rector.phar?

Copy link
Member

@TomasVotruba TomasVotruba Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In /bin only bin files should be. build is for the extra generated stuff

Don't worry, it will be usable as vendor/bin/rector.phar in the end.

"rsync -Rr --exclude-from '.pharignore' . ./build-phar/",
"bin/rector process ./build-phar/bin/ --config packages/PharBuilder/src/config/rector.yml --dry-run",
"php -dphar.readonly=0 packages/PharBuilder/bin/compile",
"rm -rf ./build-phar"
]
},
"bin": [
"bin/rector",
Expand Down
48 changes: 3 additions & 45 deletions packages/PharBuilder/src/Compiler/Compiler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@
use Phar;
use Rector\PharBuilder\Exception\BinFileNotFoundException;
use Rector\PharBuilder\Filesystem\PathNormalizer;
use Rector\PharBuilder\Filesystem\PharFilesFinder;
use Rector\PharBuilder\FinderToPharAdder;
use Seld\PharUtils\Timestamps;
use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\Finder\Finder;
use Symfony\Component\Process\Process;

final class Compiler
{
Expand All @@ -20,11 +16,6 @@ final class Compiler
*/
private $pharName;

/**
* @var PharFilesFinder
*/
private $pharFilesFinder;

/**
* @var SymfonyStyle
*/
Expand All @@ -40,11 +31,6 @@ final class Compiler
*/
private $pathNormalizer;

/**
* @var FinderToPharAdder
*/
private $finderToPharAdder;

/**
* @var string
*/
Expand All @@ -54,17 +40,13 @@ public function __construct(
string $pharName,
string $binFileName,
string $buildDirectory,
PharFilesFinder $pharFilesFinder,
SymfonyStyle $symfonyStyle,
PathNormalizer $pathNormalizer,
FinderToPharAdder $finderToPharAdder
PathNormalizer $pathNormalizer
) {
$this->pharName = $pharName;
$this->pharFilesFinder = $pharFilesFinder;
$this->binFileName = $binFileName;
$this->symfonyStyle = $symfonyStyle;
$this->pathNormalizer = $pathNormalizer;
$this->finderToPharAdder = $finderToPharAdder;
$this->buildDirectory = realpath($buildDirectory);
}

Expand All @@ -77,23 +59,9 @@ public function compile(): void
$phar->setSignatureAlgorithm(Phar::SHA1);
$phar->startBuffering();

// use only dev deps + rebuild dump autoload
// $this->symfonyStyle->note('Removing dev packages from composer');
// $process = new Process('composer update --no-dev', $buildDirectory);
// $process->run();

// dump autoload
// $this->symfonyStyle->note('Dumping new composer autoload');
// $process = new Process('composer dump-autoload --optimize', $buildDirectory);
// $process->run();
$this->symfonyStyle->note(sprintf('Adding files from directory %s', $this->buildDirectory));

$finder = $this->pharFilesFinder->createForDirectory($this->buildDirectory);

$fileCount = $this->getFileCountFromFinder($finder);
$this->symfonyStyle->note(sprintf('Adding %d files', $fileCount));
$this->symfonyStyle->progressStart($fileCount);

$this->finderToPharAdder->addFinderToPhar($finder, $phar);
$phar->buildFromDirectory($this->buildDirectory);

$this->symfonyStyle->newLine(2);
$this->symfonyStyle->note('Adding bin');
Expand All @@ -106,11 +74,6 @@ public function compile(): void
$timestamps = new Timestamps($this->pharName);
$timestamps->save($this->pharName, Phar::SHA1);

// return dev deps
// $this->symfonyStyle->note('Returning dev packages to composer');
// $process = new Process('composer update', $buildDirectory);
// $process->run();

$this->symfonyStyle->success(sprintf('Phar file "%s" build successful!', $this->pharName));
}

Expand Down Expand Up @@ -146,11 +109,6 @@ private function removeShebang(string $content): string
return preg_replace('~^#!/usr/bin/env php\s*~', '', $content);
}

private function getFileCountFromFinder(Finder $finder): int
{
return count(iterator_to_array($finder->getIterator()));
}

private function ensureBinFileExists(string $binFilePath): void
{
if (file_exists($binFilePath)) {
Expand Down
35 changes: 0 additions & 35 deletions packages/PharBuilder/src/Filesystem/PharFilesFinder.php
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/PharBuilder/src/config/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ parameters:
# customizable (via %ENV()?)
pharName: 'rector.phar'
binFileName: 'bin/rector'
buildDirectory: '%kernel.root_dir%/../../../..'
buildDirectory: '%kernel.root_dir%/../../../../build-phar'

services:
_defaults:
Expand Down
7 changes: 7 additions & 0 deletions packages/PharBuilder/src/config/rector.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
parameters:
prefix: 'RectorPhar'

services:
Rector\Rector\Dynamic\NamespaceReplacerRector:
$oldToNewNamespaces:
'Jean85': '%prefix%Jean85'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very creative use of Rector, like it 👍

Love the param! :)
There could be dynamic Rector (via RectorProvider), that would replace every namespace node with prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every except specified would be even better:) We don't need to prefix Rector itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see :) That would require new Rector, probably NamespacePrefixerRector

services:
    Rector\Rector\Dynamic\NamespaceReplacerRector:
        !Rector: Prefixed # all but "Rector" prefix by "Prefixed"

What do you need to know from me to write it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will know that, after I try it:) But I feel like performance is bigger issue here, can we somehow speed it up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First things first. When PHAR is done, we can improve performance.

Doing both at same time improved neither of both, tried for you :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with Tomas here. Let's first fix this PHAR issue, than we can join forces together and make Rector as fast as we can 💪