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

Unused dependencies are not indexed #650

Open
giem-git opened this issue May 14, 2021 · 8 comments
Open

Unused dependencies are not indexed #650

giem-git opened this issue May 14, 2021 · 8 comments
Labels

Comments

@giem-git
Copy link

Description
Suppose we have a root requirement on package A.
Package A has a dependency on package B: "^1.0"
During satis build, package B v1.0 is downloaded and indexed
Later, when B v1.1 is released, the next Satis build will download and index it, but will not index B v1.0 (though dumped package is still here)

If a client has B : v1.0 in its composer.lock, composer install fails.

Cause
It seems that dumped packages are merged with new packages only if a filter has been used:

BuildCommand.php around line 200 :
if ($packageSelection->hasFilterForPackages() || $packageSelection->hasRepositoryFilter()) {
//merge old packages
}
this does not take into account when an old package is no longer required by current root packages.

I can take care of a fix if you agree with my analysis.

Proposed fixes

  1. Always merge old packages
  2. Add a configuration option 'always-merge-old-packages'
  3. Try to detect if old packages are orphaned ? (though this one seems much more difficult to implement)

Option 2 seems to me the best choice

@giem-git giem-git added the bug label May 14, 2021
@SvenRtbg
Copy link
Contributor

Please be careful: I remember that there was more than one pull request fiddling with the amount of packages that are fetched. And I was always thinking to myself "There is yet another option for a specific use case added. Who will maintain all combinatorial possibilities and test them?"

So please try to avoid adding another option that is trying to fix any interference of existing options.

Option 3 is out for obvious reasons: Satis must be able to know which packages are actually required by the clients of the repository. There is no way to know this just based on the configuration file.

Or maybe your analysis is incomplete. Do you have a test case? You'd need it anyways during implementation of the fix.

@giem-git
Copy link
Author

Hello,

I understand your concern about another pull request fiddling with the amount of packages fetched.
Here it's not exactly the case : the packages fetched are exactly the same, but we ensure
packages that where once fetched and no longer are (because new versions have been released) are
still indexed in our repository's packages.json

Test case

Summary

This test case allows to pinpoint the bug.

  • step 1a : Build a repository with a root package package_a and a dependency package_b in release 1.0
  • step 1b : have a client project require package_a and build a composer.lock file
  • step 2a : release package b v1.1 and rebuild repository : package b v1.0 is no longer present in the repository index
  • step 2b : launch composer upgrade --lock on client. Composer 2 fails (we did not have the problem with composer 1)

if we make sure that old packages are indexed by satis, as described in my previous post, composer2 upgrade --lock works just fine.

Step 1a - build repository with package_a 1.0 and package_b 1.0

For the need of this test case, we fake packages and versions by using 'package' type repositories.

Our satis server config :

{
    "name": "tests/satis",
    "homepage": "http://127.0.0.1/satis",
    "repositories": [
        {
            "type": "package",
            "package" : {
                "name" : "tests/package_a",
                "version" : "1.0.0",
                "require": {
                   "tests/package_b": "^1"
                },
                "dist" : {
                    "url": "https://github.com/composer/satis/archive/refs/tags/1.0.0.zip",
                    "type": "zip"
                }
            }
        },
        {
            "type": "package",
            "package" : {
                "name" : "tests/package_b",
                "version" : "1.0",
                "dist" : {
                    "url": "https://github.com/composer/satis/archive/refs/tags/1.0.0.zip",
                    "type": "zip"
                }
            }
        }
    ],
    "require": {
        "tests/package_a": "^1.0"
    },
    "require-all": false,
    "require-dependencies": true,
    "require-dev-dependencies": true,
    "archive": {
        "directory": "dist",
        "format": "zip"
    }
}

We build the satis repository using this config file :

$ bin/satis build test_case.json web/
Scanning packages
Creating local downloads in 'web//dist'
Dumping package 'tests/package_a' in version '1.0.0'.
  - Installing tests/package_a (1.0.0): Downloading (100%)
Dumping package 'tests/package_b' in version '1.0'.
  - Installing tests/package_b (1.0): Downloading (100%)
Wrote packages to web//include/all$85b40c1baad52bea4f7373171f7048919ddd381f.json
Wrote packages to web//p2/tests/package_a.json
Wrote packages to web//p2/tests/package_a~dev.json
Wrote packages to web//p2/tests/package_b.json
Wrote packages to web//p2/tests/package_b~dev.json
Writing packages.json
Pruning include directories
Writing web view

step 1b - install client and generate a composer.lock file

On the client side, we initiate a blank project with this Composer.json :

{
  "repositories": {
    "tests": {
      "type": "composer",
      "url": "http://127.0.0.1/satis"
    },
    "packagist": false
  },
  "require": {
    "tests/package_a": "^1"
  },
  "config": {
    "secure-http": false
  }
}
$ composer2 install
No lock file found. Updating dependencies instead of installing from lock file. Use composer update over composer install if you do not have a lock file.
Loading composer repositories with package information
Warning: Accessing 127.0.0.1 over http which is an insecure protocol.
Updating dependencies
Lock file operations: 2 installs, 0 updates, 0 removals
  - Locking tests/package_a (1.0.0)
  - Locking tests/package_b (1.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 2 installs, 0 updates, 0 removals
  - Downloading tests/package_b (1.0)
  - Downloading tests/package_a (1.0.0)
  - Installing tests/package_b (1.0): Extracting archive
  - Installing tests/package_a (1.0.0): Extracting archive
Generating autoload files

So far so good :)

steb 2a - Release package_b version 1.1

We now simulate the release of version 1.1 of package_b by inserting this block in the repositories section of our config :

        {
            "type": "package",
            "package" : {
                "name" : "tests/package_b",
                "version" : "1.1",
                "dist" : {
                    "url": "https://github.com/composer/satis/archive/refs/tags/1.0.0.zip",
                    "type": "zip"
                }
            }
        },

and we build our satis repo :

$ bin/satis build test_case.json web/
Scanning packages
Creating local downloads in 'web//dist'
Dumping package 'tests/package_a' in version '1.0.0'.
Dumping package 'tests/package_b' in version '1.1'.
  - Installing tests/package_b (1.1): Downloading (100%)
Wrote packages to web//include/all$ed90c0a7815fb51f0c8907be95f2498b32790222.json
Wrote packages to web//p2/tests/package_a.json
Wrote packages to web//p2/tests/package_a~dev.json
Wrote packages to web//p2/tests/package_b.json
Wrote packages to web//p2/tests/package_b~dev.json
Writing packages.json
Pruning include directories
Deleted web//include/all$85b40c1baad52bea4f7373171f7048919ddd381f.json
Writing web view

Remark : package_b version 1.0 is no longer indexed by our repository. .

steb 2b - composer upgrade --lock fails

On the client side :

composer2 update --lock
Loading composer repositories with package information
Warning: Accessing 127.0.0.1 over http which is an insecure protocol.
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires tests/package_b == 1.0.0.0, it is satisfiable by tests/package_b[1.0] from lock repo but tests/package_b[1.1] from composer repo (http://127.0.0.1/satis) has higher repository priority. The packages with higher priority do not match your constraint and are therefore not installable. See https://getcomposer.org/repoprio for details and assistance.
  Problem 2
    - Root composer.json requires tests/package_a == 1.0.0.0 -> satisfiable by tests/package_a[1.0.0].
    - tests/package_a 1.0.0 requires tests/package_b ^1 -> found tests/package_b[1.1] but these were not loaded, likely because it conflicts with another require.

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.
  • This behaviour is new with composer 2.0, it did not occure when we were using composer 1.0
  • if we make sure that old packages are indexed by satis, as described in my previous post, composer2 upgrade --lock works just fine.

Hope I'm clear and not too long in this response.
Thanks in advance

@stof
Copy link
Contributor

stof commented May 20, 2021

making the output of a build depending on the existing build output looks wrong to me. It makes build non reproducible.

If you want to include all matching versions of the dependencies instead of only the one that gets selected by the composer version selection algorithm, use require-dependency-filter: false. this way, older releases are kept

@mikaelmeulle
Copy link

mikaelmeulle commented May 20, 2021

this is an interesting thread.
First, let's note that, when using a package filter or a repository filter, history is kept, not matterthe previous options or treatments (line 202 in src/Console/Command/BuildCommand.php)

        if ($packageSelection->hasFilterForPackages() || $packageSelection->hasRepositoryFilter()) {
            // in case of an active filter we need to load the dumped packages.json and merge the
            // updated packages in
            $oldPackages = $packageSelection->load();
            $packages += $oldPackages;
            ksort($packages);
        }

Second, I think here we are trying to find an intermediate solution between

  1. "keep everything" (require-dependency-filter: false + only-best-candiates: false),
  2. "only necessary" (require-dependency-filter: true + only-best-candiates: true)

Why would we let package dependencies disappear from build to build if they still match the constraints of satis.json and have been previously used by clients of the satis server?

Indeed, In my humble opinion of a PHP developper running a SATIS server inside a large public organization, it is important for a SATIS server to allow clients to build properly over time without forcing them to update their composer.lock file every time a dependency of a satis' root package has released a new version...

i hope i am clear in my explanations.
To summarize, an option in SATIS to keep history would benefit our usage of SATIS inside our organization.
I would be happy to help and contribute.

@giem-git
Copy link
Author

When our clients where using composer 1 we where fine with the current behaviour. Clients having a composer.lock integrated smoothly with our satis repository and could launch composer install or composer update --lock without errors.

Today with composer2 this is no longer the case. Maybe we have the wrong approach and it's rather a composer2 bug ?

Annyway, having satis index old packages solves our problem.

As mentionned by @mikaelmeulle satis already merges old dumps on certain situations...
Issue #556 mentions this but would rather have satis not merging old packages when filtering...
Maybe the best would be to let user choose whether to merge or not old packages with an option :

merge-old-packages = [yes|no|auto] 

with auto beeing the default and corresponding to current satis behavior (merge only when filtering)

@fasterforward
Copy link

We ran against the same situation. Is there a solution?

@fasterforward
Copy link

Is there any progress on this issue?

@alcohol alcohol added feature and removed bug labels Jul 4, 2023
@alcohol
Copy link
Member

alcohol commented Jul 4, 2023

I do not see any open pull requests referencing this issue.

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

6 participants