ort icon indicating copy to clipboard operation
ort copied to clipboard

PHP Composer install issues

Open JorisVanEijden opened this issue 3 years ago • 6 comments

The current PhpComposer manager does this for every composer.json file found:

  1. Rename an existing "vendor" directory.
  2. Fail to step 5 if a corresponding composer.lock file does not exist and allowDynamicVersions is false.
  3. Try to run composer install and fail to step 5 on error.
  4. Parse the composer.lock file for dependencies.
  5. Delete vendor directory and restore any vendor directory renamed in step 1.

Composer install uses the composer.lock if it exists, and creates it from the composer.json if it does not exist. Therefore running composer.install when a composer.lock file already exists does not make sense.

Unless you want to scan the installed dependencies for non-composer dependencies, like npm packages. But then you should not delete the vendor directory after install.

My proposal is:

  1. Do not stash/unstash vendor directory unless you're going to do an install
  2. Do an install only if analyzerConfig.allowDynamicVersions is true and a composer.lock file does not exist
  3. Make the presence of a compose.json without a composer.lock when allowDynamicVersions is false, a Warning in stead of an Error.

Benefits:

  • Faster analysis
  • No more ignored composer.lock dependencies when composer install fails (which can happen for many reasons)
  • No more pollution from composer installs that install to other directories than "vendor"

Drawbacks:

  • The benefits do not apply when allowDynamicVersions is true and there is a composer.json without a composer.lock present

JorisVanEijden avatar Jan 18 '22 10:01 JorisVanEijden

Rather than doing a composer install to generate a composer.lock file to analyze, with composer version 2 you can do a composer update --no-install to create just a composer.lock file without installing anything.

JorisVanEijden avatar Jan 19 '22 08:01 JorisVanEijden

Thanks for this through proposal, @JorisVanEijden! Unfortunately, we cannot foresee yet when one of us will have the time to implement it.

sschuberth avatar Feb 23 '22 21:02 sschuberth

I would also be interested in helping on this topic, but I can't code in Kotlin. So I was wondering if any small steps would be useful, like starting with replacing composer version 1 currently in the default docker image with composer version 2.

camillem avatar Jun 22 '22 09:06 camillem

starting with replacing composer version 1 currently in the default docker image with composer version 2.

Unfortunately, the latest Composer version in Ubuntu Focal (which we currently base our Docker image on) is version 1.10.1-1, which is the version we're already using. We probably could install Composer from source, but that's then more involving.

~But then again upgrading the Composer version was not part of @JorisVanEijden plan anyway, it seems.~

Scratch that, I missed the follow-up comment:

with composer version 2 you can do a composer update --no-install to create just a composer.lock file without installing anything.

sschuberth avatar Jun 22 '22 15:06 sschuberth

We probably could install Composer from source, but that's then more involving.

Yes, that's what I'm currently doing and it seems to work fine, but I have not thoroughly tested yet. I have also added "composer-runtime-api" to EXCLUDED_PACKAGES in analyzer/src/main/kotlin/managers/Composer.kt (cf. doc )

Is it worth me testing it more and trying to make a PR or would the signal/noise ratio be too low?

camillem avatar Jun 22 '22 18:06 camillem

Is it worth me testing it more and trying to make a PR or would the signal/noise ratio be too low?

That depends on the quality of the PR 😄 Just kidding... sure, please file a PR!

sschuberth avatar Jun 22 '22 18:06 sschuberth