phar-composer icon indicating copy to clipboard operation
phar-composer copied to clipboard

Do not add require-dev packages by default

Open lsv opened this issue 9 years ago • 9 comments

So I made a simple "hack" so it only adding requires and not require-devs.

I read in one of the other issues (actually this was a PR #50 ), that it was a good thing to use composer/installed.json because this also had verions.

So I took the best of both worlds.

First load the required package into a list

        // Load composer.lock requires
        $requiredPackages = [];
        if (is_file($pathVendor . '../composer.lock')) {
            $compoaserPackages = $this->loadJson($pathVendor . '../composer.lock');
            foreach($compoaserPackages['packages'] as $pack) {
                $requiredPackages[$pack['name']] = true;
                if (isset($pack['require'])) {
                    foreach($pack['require'] as $key => $tmp)
                    $requiredPackages[$key] = true;
                }
            }
        }

And inside the loader of the getPackagesDependencies method just add 3 lines to the installed.json loader.

So getPackagesDependencies in my version looks like this (The loader for the required packages should properly be put into a private method)

public function getPackagesDependencies()
    {
        $packages = array();

        $pathVendor = $this->getPathVendor();

        // Load composer.lock requires
        $requiredPackages = [];
        if (is_file($pathVendor . '../composer.lock')) {
            $compoaserPackages = $this->loadJson($pathVendor . '../composer.lock');
            foreach($compoaserPackages['packages'] as $pack) {
                $requiredPackages[$pack['name']] = true;
                if (isset($pack['require'])) {
                    foreach($pack['require'] as $key => $tmp)
                    $requiredPackages[$key] = true;
                }
            }
        }

        // load all installed packages (use installed.json which also includes version instead of composer.lock)
        if (is_file($pathVendor . 'composer/installed.json')) {
            // file does not exist if there's nothing to be installed
            $installed = $this->loadJson($pathVendor . 'composer/installed.json');

            foreach ($installed as $package) {
                if (! isset($requiredPackages[$package['name']])) {
                    continue;
                }

                $dir = $package['name'] . '/';
                if (isset($package['target-dir'])) {
                    $dir .= trim($package['target-dir'], '/') . '/';
                }

                $dir = $pathVendor . $dir;
                $packages [] = new Package($package, $dir);
            }
        }

        return $packages;
    }

For mine 2 projects this actually works perfect, and gives me a 1/3 sized phar file.

Are you interested in a PR ?

lsv avatar Feb 11 '16 13:02 lsv

Thanks for posting this issue, I'll use this as a base to link all related PR against :+1:

I think we all agree that it makes sense to NOT add require-dev packages by default.

While this feature may look easy to implement, it actually has some edge cases we should consider. This has been discussed in some of the open PRs before, so I'll post some of the discussion here for the reference:

Upon further inspection I think we should consider a few more things (just listing some random thoughts):

  • Running composer install --no-dev and then running phar-composer returns the expected phar with no dev dependencies installed (arguably a bit cumbersome, but explicit)
  • Running composer install (the default) and then running phar-composer returns a phar with dev dependencies installed (arguably not expected, but in line with how composer works)
  • The composer.lock is identical in both cases, whereas the installed.json only lists packages actually installed
  • Running phar-composer install already runs composer install --no-dev
  • If we install with dev dependencies but only pick non-dev dependencies, then the autoloader still contains dev dependencies (albeit unlikely, this could possibly cause conflicts)
  • What if we run in a fresh repository with only a composer.json, no composer.lock and no vendor/ (often the case with libraries)?
  • What if we run in a fresh repository with both a composer.json and composer.lock and no vendor/ (often the case with application projects)?

As an alternative approach, we may also look into merely detecting if dev dependencies have been installed. We could then either ask the user to confirm or abort or in the future also suggest installing into a temporary directory without dev dependencies.

Any thoughts?

clue avatar Jun 29 '16 12:06 clue

  • What if we run in a fresh repository with only a composer.json, no composer.lock and no vendor/ (often the case with libraries)?
  • What if we run in a fresh repository with both a composer.json and composer.lock and no vendor/ (often the case with application projects)?

These are bogus, obviously :) phar-composer will complain if there's no vendor/ directory, as this means there's also no autoloader available.

If we install with dev dependencies but only pick non-dev dependencies, then the autoloader still contains dev dependencies (albeit unlikely, this could possibly cause conflicts)

I think this is the main concern here. Is this really going to cause any issues? We could perhaps consider adding a warning if we detect a dev-dependency has been installed (present in installed.json).

clue avatar Jul 15 '16 16:07 clue

@radford raised the question whether this whole change should be considered a BC break (and thus a bump to v2.0) or if we see this as a pure feature addition (v1.1 release).

clue avatar Jul 18 '16 11:07 clue

well, if it works differently from previous version - then yes - it's BC and major bump should be in place. I just see how people don't want to bump major versions, like it's big difference its version 1 or 2. Another plus for BC break/major bump is that u can pack additional BC into v2

shivas avatar Jul 26 '16 15:07 shivas

@radford raised the question whether this whole change should be considered a BC break (and thus a bump to v2.0)

Definitely BC – if --no-dev becomes the default.

It breaks projects relying on the old behavior, e.g.:

$ php my.phar test # run unit tests of my.phar

schnittstabil avatar Sep 21 '16 21:09 schnittstabil

Thanks for your feedback! In my opinion, not including dev dependencies should be the default as it applies to 95% of use cases. I do not consider this to be a BC break because this is not currently part of this project's BC promise.

Existing phars that include any dev dependencies will continue to work as usual. New phars created with the future version will not include them by default anymore. If you need development dependencies, we will add a new --dev flag to explicitly add them.

This way, the default behavior is sane for most consumers and the behavior is consistent throughout all commands (install vs build). On top of this, this is important because including this project as a dev dependency is becoming increasingly common and you usually don't want to include your build tools in your build artifact.

clue avatar Nov 08 '19 08:11 clue

For the reference: At the moment, this project will build whatever is installed in the local project directory. This means one can also run composer install --no-dev before running the build command to exclude any dev dependencies (see also https://github.com/clue/graph-composer/pull/44):

$ composer install --no-dev && phar-composer build . && composer install

This does work just fine if this project is not installed as a dev dependency itself. If so, the project source needs to be copied to a temporary build directory first.

clue avatar Nov 08 '19 08:11 clue

Any updates on this? I'm new to PHARing my php files, and I started using this tool to make life easier. I've encountered this issue. Noticed that all the tools used for my units tests, like phpunit, are included in the phar. It should definitely be taken up or allow some way to have phar-composer support a --no-dev option if you're worried about defaulting and breaking changes. But quite honestly, it should have behaved as the author metioned... without dev to start with.

superflyxxi avatar Oct 18 '21 01:10 superflyxxi

Any updates on this?

See the possible solution above. As much as I'd love to work on an easier solution myself, there are currently no immediate plans to build this from my end (no demand at the moment and more important outstanding issues currently). If you need this for a commercial project and you want to help sponsor this feature, feel free to reach out and I'm happy to take a look.

clue avatar Oct 20 '21 13:10 clue