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.

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

composer update packages on remove a package #8870

Closed
vnagara opened this issue May 4, 2020 · 13 comments
Closed

composer update packages on remove a package #8870

vnagara opened this issue May 4, 2020 · 13 comments
Assignees
Labels
Milestone

Comments

@vnagara
Copy link

vnagara commented May 4, 2020

Update: Next my comment describes how to reproduce it easier:

Cross it out: I tried to reduce composer to minimal version to reproduce bug and unfortunately the bug doesn't reveal. I have to add my lock file to reproduce the bug as it very tricky.

Prerequisite: download composer.lock (https://gist.github.com/vnagara/1b097e6e941e7fae724c6b4619743c33) and do composer install first.

My composer.json:

{
    "name": "symfony/skeleton",
    "type": "project",
    "license": "MIT",
    "description": "A minimal Symfony project recommended to create bare bones applications",
    "require": {
        "php": "^7.2.5",
        "ext-ctype": "*",
        "ext-iconv": "*",
        "symfony/apache-pack": "^1.0",
        "symfony/console": "*",
        "symfony/dom-crawler": "5.0.*",
        "symfony/dotenv": "*",
        "symfony/flex": "^1.3.1",
        "symfony/form": "5.0.*",
        "symfony/framework-bundle": "*",
        "symfony/security-csrf": "5.0.*",
        "symfony/twig-pack": "^1.0",
        "symfony/var-dumper": "5.0.*",
        "symfony/yaml": "*"
    },
    "require-dev": {
    },
    "config": {
        "preferred-install": {
            "*": "dist"
        },
        "sort-packages": true,
        "platform": {
            "php": "7.2.29"
        }        
    },
    "autoload": {
        "psr-4": {
            "App\\": "src/"
        }
    },
    "autoload-dev": {
        "psr-4": {
            "App\\Tests\\": "tests/"
        }
    },
    "replace": {
        "symfony/intl": "5.*",
        "paragonie/random_compat": "2.*",
        "symfony/polyfill-ctype": "*",
        "symfony/polyfill-iconv": "*",
        "symfony/polyfill-php72": "*",
        "symfony/polyfill-php71": "*",
        "symfony/polyfill-php70": "*",
        "symfony/polyfill-php56": "*"
    },
    "scripts": {
        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install %PUBLIC_DIR%": "symfony-cmd"
        },
        "post-install-cmd": [
            "@auto-scripts"
        ],
        "post-update-cmd": [
            "@auto-scripts"
        ]
    },
    "conflict": {
        "symfony/symfony": "*"
    },
    "extra": {
        "symfony": {
            "allow-contrib": false,
            "require": "5.0.*"
        }
    }
}

Output of composer diagnose:

Checking composer.json: WARNING
require.symfony/console : unbound version constraints (*) should be avoided
require.symfony/dotenv : unbound version constraints (*) should be avoided
require.symfony/framework-bundle : unbound version constraints (*) should be avoided
require.symfony/yaml : unbound version constraints (*) should be avoided
Checking platform settings: OK
Checking git settings: OK
Checking http connectivity to packagist: OK
Checking https connectivity to packagist: OK
Checking github.com rate limit: OK
Checking disk free space: OK
Checking pubkeys: 
Tags Public Key Fingerprint: 57815BA2 7E54DC31 7ECC7CC5 573090D0  87719BA6 8F3BB723 4E5D42D0 84A14642
Dev Public Key Fingerprint: 4AC45767 E5EC2265 2F0C1167 CBBB8A2B  0C708369 153E328C AD90147D AFE50952
OK
Checking composer version: You are not running the latest stable version, run `composer self-update` to update (1.10.1 => 1.10.5)
Composer version: 1.10.1
PHP version: 7.2.29 - Package overridden via config.platform (actual: 7.4.5)
PHP binary path: /usr/bin/php7.4
OpenSSL version: OpenSSL 1.1.1c  28 May 2019

When I run this command:

composer require composer/composer
composer remove composer/composer

I get the following output:

Dependency "symfony/console" is also a root requirement, but is not explicitly whitelisted. Ignoring.
Loading composer repositories with package information
Updating dependencies (including require-dev)
Restricting packages listed in "symfony/symfony" to "5.0.*"
Package operations: 0 installs, 2 updates, 9 removals
  - Removing symfony/process (v5.0.8)
  - Removing seld/phar-utils (1.1.0)
  - Removing seld/jsonlint (1.8.0)
  - Removing justinrainbow/json-schema (5.2.9)
  - Removing composer/xdebug-handler (1.4.1)
  - Removing composer/spdx-licenses (1.5.3)
  - Removing composer/semver (1.5.1)
  - Removing composer/composer (1.10.5)
  - Removing composer/ca-bundle (1.2.7)
  - Updating symfony/filesystem (v5.0.7 => v5.0.8): Loading from cache
  - Updating symfony/finder (v5.0.7 => v5.0.8): Loading from cache
Writing lock file
Generating autoload files

And I expected to NOT HAVE this lines:

  - Updating symfony/filesystem (v5.0.7 => v5.0.8): Loading from cache
  - Updating symfony/finder (v5.0.7 => v5.0.8): Loading from cache

And it for composer outdated there another 10 packages that wasn't updated. Therefore it updates only partially. However I expect packages as they were before, no updates unless I explicitly asked for it.
Plus as side effect symfony.lock doesn't reflect this update either and shows symfony/filesystem v5.0.7.

@stof
Copy link
Contributor

stof commented May 4, 2020

Currently, the remove command whitelists dependencies of the removed package, to allow them to be removed (otherwise, they would be kept). But currently, this also allows changing their versions.

@naderman is it possible that the new architecture of the solver in Composer 2 will solve this, by fixing the version of all components without forcing them to be installed ? That would solve the case of remove, as it could fix all package versions while still removing unused dependencies.

@vnagara
Copy link
Author

vnagara commented May 4, 2020

Figured out how to reproduce it easier:

To make it smaller and imitate the outdated 2 packages:

composer.json:

{
    "require": {
        "symfony/filesystem": "5.0.7",    
        "symfony/finder": "5.0.7",    
        "symfony/framework-bundle": "^5.0"
    }
}

Commands:

composer install

Remove 2 lines from composer.json:

{
    "require": {
        "symfony/framework-bundle": "^5.0"
    }
}

Run this command:

composer require composer/composer
composer remove composer/composer

I get the following output:

Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 2 updates, 10 removals
  - Removing symfony/process (v5.0.8)
  - Removing symfony/console (v5.0.8)
  - Removing seld/phar-utils (1.1.0)
  - Removing seld/jsonlint (1.8.0)
  - Removing justinrainbow/json-schema (5.2.9)
  - Removing composer/xdebug-handler (1.4.1)
  - Removing composer/spdx-licenses (1.5.3)
  - Removing composer/semver (1.5.1)
  - Removing composer/composer (1.10.5)
  - Removing composer/ca-bundle (1.2.7)
  - Updating symfony/filesystem (v5.0.7 => v5.0.8): Loading from cache
  - Updating symfony/finder (v5.0.7 => v5.0.8): Loading from cache
Writing lock file

And I not expect to have

  - Updating symfony/filesystem (v5.0.7 => v5.0.8): Loading from cache
  - Updating symfony/finder (v5.0.7 => v5.0.8): Loading from cache

@Seldaek
Copy link
Member

Seldaek commented May 4, 2020

Yeah this is completely expected in v1.. And not fixable there, unless you do composer remove composer/composer --no-update-with-dependencies.

In v2 I think it might be fixable as we have way better control over the pool via poolbuilder. I believe fixed packages are required to be present so that wouldn't really be an option, but either we add a concept of soft-fixed package which must be at version X or gone, or we could add a second pass to the update which would first remove the packages you ask to remove, and then run the equivalent of a composer update --unused which removes unused ones, which was the whole point of whitelisting the dependencies of the removed package.

I think a better alternative might be a new setUpdateAllowTransitiveDependencies mode for the Installer, smth like REMOVE_LISTED_WITH_TRANSITIVE_DEPS which would be the default for composer remove if no --with-dependencies or whatever is passed. And it would do like UPDATE_LISTED_WITH_TRANSITIVE_DEPS, but would only execute remove operations on the lock file.. Any other operation would be ignored, so we'd by default only cleanup unused packages resulting of the removal, but if you do --with-dependencies it would switch to actually doing a proper update of these, which might allow upgrade/downgrade of dependents as the requirements of the removed package are not present anymore. IMO this is a better way as it keeps the full range of options, and does the most obvious removal-of-stuff-only by default.

@Seldaek Seldaek added this to the 2.0-core milestone May 4, 2020
@stof
Copy link
Contributor

stof commented May 4, 2020

@Seldaek I think this could be solved, by ensuring that fixed package have only that version in the pool (so that they cannot change version), but without forcing to add a root requirement for them in the solver (so the solver would not select them if they are unused, leading to their removal). Removing unused dependencies should not require an extra step in the new architecture IMO.

@Seldaek
Copy link
Member

Seldaek commented May 4, 2020 via email

@vnagara
Copy link
Author

vnagara commented May 4, 2020

@Seldaek @stof I should point out that it doesn't update those packages on require composer/composer but specifically on remove that is even more strange.

@stof
Copy link
Contributor

stof commented May 4, 2020

@vnagara See my comment, I'm really talking about remove (which still performs a partial update internally, as it needs to update the lock file to account for the removed dependency)

@ocean90
Copy link

ocean90 commented Sep 15, 2020

In #9207 I have documented another issue with the johnpbloch/wordpress package where the remove command now has a different behaviour in v2, the dependencies are no longer removed.

@vnagara vnagara changed the title composer update packages on remove package composer update packages on remove a package Sep 15, 2020
@naderman
Copy link
Member

@ocean90 @vnagara can you try with this PR applied: #9223

@vnagara
Copy link
Author

vnagara commented Sep 17, 2020

Here what I have

$ composer/bin/composer require composer/composer
Using version ^1.10 for composer/composer
./composer.json has been updated
Running composer update composer/composer
Loading composer repositories with package information
Updating dependencies
Lock file operations: 13 installs, 0 updates, 0 removals
  - Locking composer/ca-bundle (1.2.8)
  - Locking composer/composer (1.10.13)
  - Locking composer/semver (1.7.0)
  - Locking composer/spdx-licenses (1.5.4)
  - Locking composer/xdebug-handler (1.4.3)
  - Locking justinrainbow/json-schema (5.2.10)
  - Locking seld/jsonlint (1.8.2)
  - Locking seld/phar-utils (1.1.1)
  - Locking symfony/console (v5.1.5)
  - Locking symfony/polyfill-intl-grapheme (v1.18.1)
  - Locking symfony/polyfill-intl-normalizer (v1.18.1)
  - Locking symfony/process (v5.1.5)
  - Locking symfony/string (v5.1.5)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 13 installs, 0 updates, 0 removals
  - Downloading symfony/process (v5.1.5)
  - Downloading symfony/polyfill-intl-normalizer (v1.18.1)
  - Downloading symfony/polyfill-intl-grapheme (v1.18.1)
  - Downloading symfony/string (v5.1.5)
  - Downloading symfony/console (v5.1.5)
  - Downloading seld/jsonlint (1.8.2)
  - Downloading composer/semver (1.7.0)
  - Downloading composer/composer (1.10.13)
  - Installing symfony/process (v5.1.5): Extracting archive
  - Installing symfony/polyfill-intl-normalizer (v1.18.1): Extracting archive
  - Installing symfony/polyfill-intl-grapheme (v1.18.1): Extracting archive
  - Installing symfony/string (v5.1.5): Extracting archive
  - Installing symfony/console (v5.1.5): Extracting archive
  - Installing seld/phar-utils (1.1.1): Extracting archive
  - Installing seld/jsonlint (1.8.2): Extracting archive
  - Installing justinrainbow/json-schema (5.2.10): Extracting archive
  - Installing composer/xdebug-handler (1.4.3): Extracting archive
  - Installing composer/spdx-licenses (1.5.4): Extracting archive
  - Installing composer/semver (1.7.0): Extracting archive
  - Installing composer/ca-bundle (1.2.8): Extracting archive
  - Installing composer/composer (1.10.13): Extracting archive

And here is the problem:

$ composer/bin/composer remove composer/composer
./composer.json has been updated
Running composer update composer/composer
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 0 updates, 1 removal
  - Removing composer/composer (1.10.15)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 0 updates, 1 removal
  - Removing composer/composer (1.10.15)
Generating autoload files

It does update packages, but as you can see it leaves a lot of
packages that should be removed like on v1:

  - Removing symfony/string (v5.1.5)
  - Removing symfony/process (v5.1.5)
  - Removing symfony/polyfill-intl-normalizer (v1.18.1)
  - Removing symfony/polyfill-intl-grapheme (v1.18.1)
  - Removing symfony/console (v5.1.5)
  - Removing seld/phar-utils (1.1.1)
  - Removing seld/jsonlint (1.8.2)
  - Removing justinrainbow/json-schema (5.2.10)
  - Removing composer/xdebug-handler (1.4.3)
  - Removing composer/spdx-licenses (1.5.4)
  - Removing composer/semver (1.7.0)
  - Removing composer/composer (1.10.13)
  - Removing composer/ca-bundle (1.2.8)

@Seldaek
Copy link
Member

Seldaek commented Oct 14, 2020

Fixed by #9223

@Seldaek Seldaek closed this as completed Oct 14, 2020
@vnagara
Copy link
Author

vnagara commented Oct 14, 2020

@Seldaek @ocean90 Hi, it seems fixes updating, however it introduces new bug of leaving a lot of installed packages after removing the package that required installing them. May be I need to create a new bug for that now?

@Seldaek
Copy link
Member

Seldaek commented Oct 14, 2020

@vnagara hm can you provide an example of what you mean? with a few commands to reproduce?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants