Feature dist #804

Closed
wants to merge 45 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

till commented Jun 15, 2012

Currently downloads source and then tries to package it up as a zip.

Doesn't have a --dump command or anything yet.

Wanted some feedback on it before I move to tar.

@stof stof commented on an outdated diff Jun 16, 2012

src/Composer/Package/Dumper/BaseDumper.php
+
+/**
+ * @author Till Klampaeckel <till@php.net>
+ */
+class BaseDumper
+{
+ /**
+ * Format: zip or tarball.
+ * @var string
+ */
+ protected $format = '';
+
+ /**
+ * @var array
+ */
+ static $keys = array(
@stof

stof Jun 16, 2012

Contributor

missing visibility

@stof stof and 2 others commented on an outdated diff Jun 16, 2012

src/Composer/Package/Dumper/BaseDumper.php
+ /**
+ * @var \Composer\Util\ProcessExecutor
+ */
+ protected $process;
+
+ /**
+ * Working directory.
+ * @var string
+ */
+ protected $temp;
+
+ /**
+ * @param mixed $path
+ * @param \Composer\Util\ProcessExecutor|null $process
+ *
+ * @return \Composer\Package\Dumper\BaseDumper
@stof

stof Jun 16, 2012

Contributor

@return for a constructor ?

@till

till Jun 17, 2012

Contributor

A __construct() typically returns the class.

@stof

stof Jun 17, 2012

Contributor

no. the method itself does not.

@Seldaek

Seldaek Jun 24, 2012

Owner

I agree, if any tool in your toolchain is too retarded to figure out a constructor returns an instance, you should use another one. Useles docblocks are annoying.

@stof stof and 2 others commented on an outdated diff Jun 16, 2012

src/Composer/Package/Dumper/BaseDumper.php
+ */
+ public function getFilename(PackageInterface $package, $extension)
+ {
+ $name = preg_replace('#[^a-z0-9_-]#', '-', $package->getUniqueName());
+ $fileName = sprintf('%s.%s',
+ $name,
+ $extension
+ );
+ return $fileName;
+ }
+
+ /**
+ * @param \Composer\Package\PackageInterface $package
+ * @param string $workDir
+ */
+ protected function downloadGit(PackageInterface $package, $workDir)
@stof

stof Jun 16, 2012

Contributor

why making a separate method for each type ? Simply use the DownloadManager which is responsible to handle the different downloaders

@till

till Jun 17, 2012

Contributor

It seems like I have to configure the DownloadManager anyway. At least I can't figure out if I can pull an instance from somewhere else.

When I do the following:

$manager = new DownloadManager(true);
$manager->download($package);

I get an exception that there is no downloader for type 'git'. I don't see the point configuring another object first?

Do you know what I mean?

@Seldaek

Seldaek Jun 24, 2012

Owner

Factory::createDownloadManager might help

@stof stof and 3 others commented on an outdated diff Jun 16, 2012

src/Composer/Package/Dumper/DumperInterface.php
+ * (c) Nils Adermann <naderman@naderman.de>
+ * Jordi Boggiano <j.boggiano@seld.be>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+namespace Composer\Package\Dumper;
+
+use Composer\Package\PackageInterface;
+
+/**
+ * @author Till Klampaeckel <till@php.net>
+ */
+interface DumperInterface
+{
+ public function dump(PackageInterface $package);
@stof

stof Jun 16, 2012

Contributor

missing phpdoc for the interface, to know what the return type is

@till

till Jun 17, 2012

Contributor

It's mixed anyway. The ArrayDumper returns an array, while the others create an archive.

@palex-fpt

palex-fpt Jun 26, 2012

It is LSP violation. Every instance of DumperInterface should return array or every should return void.

As DumperInterace client how would I know is result available or not? Instanceof check?

@till

till Jun 26, 2012

Contributor

No idea.

@till

till Jun 26, 2012

Contributor

I thought about this on the way to work – so for the sake of clean code we could add a dumpFile() method to the interface which tar and zip use instead. And dump() could then throw a domain exception, and vice versa for the ArrayDumper. Just not sure if this is really worth it to be totally honest.

I think it may go over the top. ;)

@Seldaek

Seldaek Jun 26, 2012

Owner

We just don't need the interface IMO. The file dumpers are one thing,
the array dumper another. Different things, different purposes.

@stof stof and 1 other commented on an outdated diff Jun 16, 2012

src/Composer/Package/Dumper/ZipDumper.php
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Composer\Package\Dumper;
+
+use Composer\Package\Dumper\BaseDumper;
+use Composer\Package\Dumper\DumperInterface;
+use Composer\Package\PackageInterface;
+use Composer\Util\ProcessExecutor;
+
+/**
+ * @author Till Klampaeckel <till@php.net>
+ */
+class ZipDumper extends BaseDumper implements DumperInterface
@stof

stof Jun 16, 2012

Contributor

you should declare the BaseDumper class abstract and implement the DumperInterface on it to avoid doing it everywhere

@till

till Jun 17, 2012

Contributor

Thanks! I've done this. Will update the PR in a bit.

@stof stof commented on an outdated diff Jun 16, 2012

src/Composer/Package/Dumper/ZipDumper.php
+ * @author Till Klampaeckel <till@php.net>
+ */
+class ZipDumper extends BaseDumper implements DumperInterface
+{
+ protected $format = 'zip';
+
+ public function dump(PackageInterface $package)
+ {
+ $workDir = $this->getAndEnsureWorkDirectory($package);
+
+ $fileName = $this->getFilename($package, 'zip');
+ $sourceType = $package->getSourceType();
+ $sourceRef = $package->getSourceReference();
+
+ switch ($sourceType) {
+ case 'git':
@stof

stof Jun 16, 2012

Contributor

the body of the switch should be indented

This pull request passes (merged 6820f78 into 94791ab).

@till till * make BaseDumper abstract (to force extension)
 * make BaseDumper implement Interface (makes for less code in the implementation)
7355cfd

This pull request passes (merged 7355cfd into 94791ab).

This pull request passes (merged 3b72a04 into 94791ab).

Contributor

till commented Jun 18, 2012

OK, we updated once more.

These seem completed:

  • TarDumper
  • ZipDumper

Added tests, tests run against a local git repo.

This pull request passes (merged 0f6cd73 into 94791ab).

This pull request fails (merged 592f691 into 94791ab).

Seldaek was assigned Jun 24, 2012

This pull request fails (merged 1f41da8 into 94791ab).

@till till * refactored downloading:
   * removed download*() methods
   * added dep on Composer\Factory to setup a DownloadManager instance
b6f5d30

This pull request passes (merged b6f5d30 into 94791ab).

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

src/Composer/Package/Dumper/BaseDumper.php
+
+ /**
+ * @param mixed $path
+ * @param \Composer\Util\ProcessExecutor|null $process
+ *
+ * @throws \InvalidArgumentException
+ */
+ public function __construct($path = null, ProcessExecutor $process = null)
+ {
+ if (!empty($path)) {
+ if (!is_writable($path)) {
+ throw new \InvalidArgumentException("Not authorized to write to '{$path}'");
+ }
+ $this->path = $path;
+ }
+ $this->process = ($process !== null)?$process:new ProcessExecutor;
@stof

stof Jun 25, 2012

Contributor

$this->process = $process ?: new ProcessExecutor()

@till

till Jun 25, 2012

Contributor

I'm torn on this. I mean, even my one-liner is borderline-readable.

@stof

stof Jun 25, 2012

Contributor

well, first, adding the spaces around the ternary operator makes it more readable :)

And the syntax I suggested is consistent with other places in composer

@stof stof commented on the diff Jun 25, 2012

src/Composer/Package/Dumper/BaseDumper.php
+ if (!empty($path)) {
+ if (!is_writable($path)) {
+ throw new \InvalidArgumentException("Not authorized to write to '{$path}'");
+ }
+ $this->path = $path;
+ }
+ $this->process = ($process !== null)?$process:new ProcessExecutor;
+ $this->temp = sys_get_temp_dir();
+ }
+
+ /**
+ * @return \Composer\Downloader\DownloadManager
+ */
+ public function getDownloadManager()
+ {
+ $factory = new Factory;
@stof

stof Jun 25, 2012

Contributor

you should add the parenthesis even if there is no arguments

@Seldaek

Seldaek Jun 25, 2012

Owner

That really doesn't matter..

@stof stof and 1 other commented on an outdated diff Jun 25, 2012

src/Composer/Package/Dumper/BaseDumper.php
+ $this->process = ($process !== null)?$process:new ProcessExecutor;
+ $this->temp = sys_get_temp_dir();
+ }
+
+ /**
+ * @return \Composer\Downloader\DownloadManager
+ */
+ public function getDownloadManager()
+ {
+ $factory = new Factory;
+ $dm = $factory->createDownloadManager(new NullIO());
+ return $dm;
+ }
+
+ /**
+ * @param \Composer\Package\PackageInterface $package
@stof

stof Jun 25, 2012

Contributor

you should make it shorter by using the short class name. The solution of the class name will give the right result

@till

till Jun 25, 2012

Contributor

You mean in the docblock?

@stof

stof Jun 25, 2012

Contributor

yes, on the line on which I commented

till added some commits Jun 25, 2012

@till till * update CS with feedback from @stof d244f87
@till till * ArrayDumper doesn't extend BaseDumper anymore (hence no conflict o…
…n the interface)

 * move keys from BaseDumper back to ArrayDumper
 * interface now declares dump() to always return void
a1d287c
@till till Merge remote-tracking branch 'upstream/master' into feature-dist
Conflicts:
	composer.lock
fe6bac4

This pull request fails (merged fe6bac4 into a8700fb).

This pull request fails (merged 55414ba into fa982a9).

This pull request fails (merged 7ad3185 into fa982a9).

This pull request fails (merged 926a36d into 0a55707).

@stof stof commented on an outdated diff Aug 8, 2012

src/Composer/Package/Dumper/ArrayDumper.php
+ 'extra',
+ 'installationSource' => 'installation-source',
+ 'license',
+ 'authors',
+ 'description',
+ 'homepage',
+ 'keywords',
+ 'autoload',
+ 'repositories',
+ 'includePaths' => 'include-path',
+ 'support',
+ );
+
+ /**
+ * @return array
+ */
@stof

stof Aug 8, 2012

Contributor

wrong indentation

@stof stof commented on an outdated diff Aug 8, 2012

src/Composer/Package/Dumper/ArrayDumper.php
@@ -21,25 +21,31 @@
*/
class ArrayDumper
{
+ /**
@stof

stof Aug 8, 2012

Contributor

tab vs spaces ?

@stof stof commented on an outdated diff Aug 8, 2012

src/Composer/Package/Dumper/BaseDumper.php
+
+use Composer\Package\PackageInterface;
+use Composer\Util\ProcessExecutor;
+use Composer\Downloader\GitDownloader;
+use Composer\Downloader\HgDownloader;
+use Composer\Downloader\SvnDownloader;
+use Composer\IO\NullIO;
+use Composer\Factory;
+
+/**
+ * @author Till Klampaeckel <till@php.net>
+ */
+abstract class BaseDumper implements DumperInterface
+{
+ /**
+ * Format: zip or tarball.
@stof

stof Aug 8, 2012

Contributor

the TarDumper uses tar, not tarball

@stof stof commented on an outdated diff Aug 8, 2012

src/Composer/Package/Dumper/BaseDumper.php
+ $this->process->execute($command, $output, $workDir);
+ }
+
+ /**
+ * @param string $fileName
+ * @param string $sourceRef
+ * @param string $workDir
+ */
+ protected function packageHg($fileName, $sourceRef, $workDir)
+ {
+ $format = ($this->format == 'tarball')?'tar':$this->format;
+ $command = sprintf(
+ 'hg archive --rev %s --type %s %s',
+ $sourceRef,
+ $format,
+ escapeshellarg('%s/%s', $this->path, $fileName)
@stof

stof Aug 8, 2012

Contributor

sprintf is missing

Contributor

stof commented Aug 8, 2012

@till what is the status of this PR ?

stof referenced this pull request in composer/satis Aug 8, 2012

Closed

Create dist packages #12

Contributor

till commented Aug 8, 2012

@stof I wish I knew. @Seldaek was not merging it prior to the last composer release, but I am not sure what happened since then.

I'll be happy to continue to work on it when I get a rough timeframe on when this is integrated, otherwise I am wasting cycles right now catching up with upstream on something that may or may not be included.

Contributor

stof commented Aug 8, 2012

@till since then, @Seldaek was in vacation :)
But my main question is whether the code is ready now or if it is still a WIP.

Btw, you should probably merge master into your branch (and keeping the lock file from master where it has been updated more recently) to fix the travis builds.

till added some commits Aug 13, 2012

Contributor

till commented Aug 13, 2012

@stof Well, it's been ready as far as I am concerned. I just fixed the bug you spotted and the tab issues.

Maybe @Seldaek and @naderman can take a look this week!

This pull request fails (merged a27471c into ebc0f88).

This pull request fails (merged 781d681 into 5e0b39e).

@stof stof and 2 others commented on an outdated diff Aug 19, 2012

composer.lock
},
{
"package": "symfony/process",
- "version": "v2.1.0-RC1"
+ "version": "dev-master",
+ "source-reference": "v2.1.0-RC1",
+ "commit-date": "1343512949"
@stof

stof Aug 19, 2012

Contributor

Please keep the lock file from the master branch. there should be no reason to change it here

@till

till Aug 20, 2012

Contributor

I didn't change anything on purpose. These references must have changed somewhere else.

I merged upstream and ran ./composer.phar update to ensure the latest deps are installed and my tests run – and this is what the update did.

Any idea how to resolve it? (cc @Seldaek )

@stof

stof Aug 20, 2012

Contributor

you did. ./composer.phar update does not installs the composer deps specified currently. It updates them and writes the new lock file. If you want to make sure your local setup has the right deps, you should run install

@stof

stof Aug 20, 2012

Contributor

and btw, the fact that you changed the lock file instead of having the lock file available in the master branch currently is the reason why tests are failing on travis

@till

till Aug 20, 2012

Contributor

Yeah, I realized it was the lock. I guess it changed because the deps are branches/a git-subtree split.

So just for the record – I did this: I ran ./composer.phar update after I installed a few weeks back. You're right, updating should not be necessary hence the lock file should not change.

Unless of course I added a dependency (, which I didn't do). But if I did, what would be the correct workflow then?

Anyway, I just pushed the lock from master.

@Seldaek

Seldaek Aug 20, 2012

Owner

If you add a dependency, you can do composer update <new dependency>
to update only that. Though it's currently a bit buggy if you don't have
the exact same state as the lock file when you do it.

@stof

stof Aug 20, 2012

Contributor

@till it is not the lock from master. It is an older lock from master.

@pborreli pborreli commented on the diff Aug 20, 2012

src/Composer/Package/Dumper/ZipDumper.php
+ $fileName = $this->getFilename($package, 'zip');
+ $sourceType = $package->getSourceType();
+ $sourceRef = $package->getSourceReference();
+
+ $dm = $this->getDownloadManager();
+ $dm->download($package, $workDir, true);
+
+ switch ($sourceType) {
+ case 'git':
+ $this->packageGit($fileName, $sourceRef, $workDir);
+ break;
+ case 'hg':
+ $this->packageHg($fileName, $sourceRef, $workDir);
+ break;
+ case 'svn':
+ $dir = $workDir . (substr($sourceRef, 0, 1) !== '/')?'/':'' . $sourceRef;
@pborreli

pborreli Aug 20, 2012

Contributor

i would write it

$dir = $workDir . ($sourceRef[0] === '/') ?: '/' . $sourceRef;

or even

$dir = $workDir . '/' . ltrim($sourceRef, '/');

@pborreli pborreli commented on the diff Aug 20, 2012

src/Composer/Package/Dumper/TarDumper.php
+ * file that was distributed with this source code.
+ */
+
+namespace Composer\Package\Dumper;
+
+use Composer\Package\Dumper\BaseDumper;
+use Composer\Package\Dumper\DumperInterface;
+use Composer\Package\PackageInterface;
+use Composer\Util\ProcessExecutor;
+
+/**
+ * @author Ulf Härnhammar <ulfharn@gmail.com>
+ */
+class TarDumper extends BaseDumper
+{
+ protected $format = 'tar';
@pborreli

pborreli Aug 20, 2012

Contributor

why don't use it and factorize dump method (removing duplicate code) between both dumper classes ?

@till

till Aug 20, 2012

Contributor

@pborreli If you have time, send me a pr to till/composer and I'll include your fixes.

@MattKetmo

MattKetmo Aug 20, 2012

Contributor

I will make a PR in the next few days. I've some ideas to develop ;)

@Baachi Baachi commented on the diff Aug 20, 2012

src/Composer/Package/Dumper/BaseDumper.php
+
+ /**
+ * Working directory.
+ * @var string
+ */
+ protected $temp;
+
+ /**
+ * @param mixed $path
+ * @param ProcessExecutor|null $process
+ *
+ * @throws \InvalidArgumentException
+ */
+ public function __construct($path = null, ProcessExecutor $process = null)
+ {
+ if (!empty($path)) {
@Baachi

Baachi Aug 20, 2012

That would be better :)

if  (null !== $path && !is_writeable($path)) {
    throw new \InvalidArgumentException("Not authorized to write to '{$path}'");
}

@Baachi Baachi commented on the diff Aug 20, 2012

src/Composer/Package/Dumper/ArrayDumper.php
@@ -78,7 +84,7 @@ public function dump(PackageInterface $package)
$data['suggest'] = $packages;
}
- foreach ($keys as $method => $key) {
+ foreach (self::$keys as $method => $key) {
@Baachi

Baachi Aug 20, 2012

static::$keys would be better

@till

till Aug 20, 2012

Contributor

Why?

@stof

stof Aug 20, 2012

Contributor

because you made it protected, meaning it is a potential extension point and using self does not allow extending. But as I don't see the need for an extension point on this array, and it was not there before, you could simply switch to private and keep self

@till

till Aug 20, 2012

Contributor

Maybe we just keep it as is because this code-base is still pre 1.0.0 and we can lock down the API after.

This pull request fails (merged 2d9af8c into ab38ee3).

This pull request passes (merged bfc8f3d into ab38ee3).

This pull request passes (merged dc2178b into ab38ee3).

This pull request passes (merged e928dbc into ab38ee3).

@Baachi Baachi commented on the diff Aug 21, 2012

src/Composer/Package/Dumper/TarDumper.php
+
+ $dm = $this->getDownloadManager();
+ $dm->download($package, $workDir, true);
+
+ switch ($sourceType) {
+ case 'git':
+ $this->packageGit($fileName, $sourceRef, $workDir);
+ break;
+ case 'hg':
+ $this->packageHg($fileName, $sourceRef, $workDir);
+ break;
+ case 'svn':
+ $dir = $workDir . (substr($sourceRef, 0, 1) !== '/')?'/':'' . $sourceRef;
+ $this->package($fileName, $dir, \Phar::TAR);
+ break;
+ default:
@Baachi

Baachi Aug 21, 2012

Use the tar shell command instead of throw an exception?
What do you think?

@stof

stof Aug 21, 2012

Contributor

@Baachi there is simply no other source type supported in composer currently anyway (and the installation is forced from source)

@Baachi Baachi commented on the diff Aug 21, 2012

src/Composer/Package/Dumper/BaseDumper.php
+ protected $temp;
+
+ /**
+ * @param mixed $path
+ * @param ProcessExecutor|null $process
+ *
+ * @throws \InvalidArgumentException
+ */
+ public function __construct($path = null, ProcessExecutor $process = null)
+ {
+ if (!empty($path)) {
+ if (!is_writable($path)) {
+ throw new \InvalidArgumentException("Not authorized to write to '{$path}'");
+ }
+ $this->path = $path;
+ }
@Baachi

Baachi Aug 21, 2012

Missing empty line here

@till

till Aug 22, 2012

Contributor

Explain, please?

@MattKetmo

MattKetmo Aug 22, 2012

Contributor

There is a CS rule for empty line before return ("Add a blank line before return statements, unless the return is alone inside a statement-group (like an if statement);" -- http://symfony.com/doc/current/contributing/code/standards.html).

This is not the case of this line, but the CS is not respected on other lines of the file...

@Baachi

Baachi Aug 23, 2012

My bad, sorry guys :)

Baachi commented Aug 23, 2012

@till You should check the CS issues. Maybe you can run the PHP-CS-Fixer :)

MattKetmo referenced this pull request Aug 24, 2012

Closed

Feature dist (bis) #1024

Contributor

till commented Sep 11, 2012

Closing this in favour of #1024

till closed this Sep 11, 2012

naderman referenced this pull request Feb 7, 2013

Merged

Distributions/Archives #1567

till deleted the till:feature-dist branch May 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment