Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Do not add require-dev packages by default #61

Open
lsv opened this issue Feb 11, 2016 · 9 comments
Open

Do not add require-dev packages by default #61

lsv opened this issue Feb 11, 2016 · 9 comments

Comments

@lsv
Copy link

lsv commented Feb 11, 2016

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 ?

@clue clue changed the title Dont add require-dev packs Do not add require-dev packages by default Jun 29, 2016
@clue
Copy link
Owner

clue commented Jun 29, 2016

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

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
Copy link
Owner

clue commented Jul 15, 2016

  • 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
Copy link
Owner

clue commented Jul 18, 2016

@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).

@shivas
Copy link

shivas commented Jul 26, 2016

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

@schnittstabil
Copy link

@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

@clue
Copy link
Owner

clue commented Nov 8, 2019

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
Copy link
Owner

clue commented Nov 8, 2019

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 clue/graph-composer#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 clue added this to the v1.2.0 milestone Nov 12, 2019
@clue clue modified the milestones: v1.2.0, v1.3.0 Dec 11, 2020
@superflyxxi
Copy link

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.

@clue
Copy link
Owner

clue commented Oct 20, 2021

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 clue removed this from the v1.3.0 milestone Dec 28, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

5 participants