Prevent unnecessary downloads of library packages - just update the json/lock files #2998

driskell · 2021-01-17T12:39:12Z

I was investigating slow dependabot running on some Drupal projects and came across issue #2416
Several things became clear as the cause, specifically around Drupal projects. One was that dependabot is slowed down by downloading actual packages when it does not need to.

This PR targets that by introducing a replacement for the LibraryInstaller that does not download or install anything.

Overall it shaves a few seconds off some of my local runs but might be worthwhile just for the bandwidth reductions.

driskell · 2021-01-17T12:46:16Z

Referencing #2999

jurre · 2021-01-17T13:33:22Z

Looking forward to giving this a spin! Thanks so much for digging into this @driskell

driskell · 2021-01-18T15:55:39Z

@jurre No worries - I had set it as a personal challenge to dig into what's happening!
It's most noticeable if I remember on the patching - so once it finds the update, and then applies to get the lock file - that stage is noticeably faster. However, it may still be slow for things like Drupal where we'd be waiting on if Composer think my patches are acceptable, in which case once Dependabot receives those updates to Composer it should fix the main issue.

stof · 2021-01-19T10:21:27Z

Composer v2 has a composer update --no-install feature, which allows to update the lock file without running the installation of new deps in the vendor folder. Configuring the code to run in this mode would be cleaner than replacing the LibraryInstaller IMO (and less code to maintain)

jurre · 2021-01-19T11:02:18Z

Composer v2 has a composer update --no-install feature, which allows to update the lock file without running the installation of new deps in the vendor folder. Configuring the code to run in this mode would be cleaner than replacing the LibraryInstaller IMO (and less code to maintain)

That's a good suggestion. I traced that setting back to setting $installer->setInstall(false), but I get a bunch of test failures when doing that. I don't have time to spend a lot of cycles on this right now, but if there's something else we'd need to configure that you know of I'd love to hear!

stof · 2021-01-19T11:52:00Z

That's a good suggestion. I traced that setting back to setting $installer->setInstall(false), but I get a bunch of test failures when doing that. I don't have time to spend a lot of cycles on this right now, but if there's something else we'd need to configure that you know of I'd love to hear!

I don't think there is anything else needed to configure that. And without seeing which test failure you have, I cannot give you an idea about a solution (note that even when seeing them, I don't guarantee that I will have an idea, given that I don't know the dependabot codebase, only the composer one)

driskell · 2021-01-19T12:31:13Z

That's a good suggestion. I traced that setting back to setting $installer->setInstall(false), but I get a bunch of test failures when doing that. I don't have time to spend a lot of cycles on this right now, but if there's something else we'd need to configure that you know of I'd love to hear!

I don't think there is anything else needed to configure that. And without seeing which test failure you have, I cannot give you an idea about a solution (note that even when seeing them, I don't guarantee that I will have an idea, given that I don't know the dependabot codebase, only the composer one)

Nice, that would definitely mean this PR is not needed. I did wonder though when I did this one if installers need to be installed still and loaded in case they impact the output. Would setInstall stop those installer plugins from being installed and loaded?

stof · 2021-01-19T13:11:59Z

Custom installer can change the location of the the installed package on disk. But they cannot change the version being selected (and so cannot affect the lock file).

The only plugins that can affect the lock file are the ones which are hooking into the dependency solver itself (using the pre-pool-create event), like symfony/flex for instance. And for that to work during a composer update, the plugin must be installed before running the update (so either as a global plugin, or by running a composer install first to have a vendor folder).

@Yash-Singh1

Changes since 7.7.4: https://github.com/npm/cli/blob/latest/CHANGELOG.md ## v7.10.0 (2021-04-15) ### FEATURES * [`f9b639eb6`](npm/cli@f9b639e) [#3052](npm/cli#3052) feat(bugs): fall back to email if provided ([@Yash-Singh1](https://github.com/Yash-Singh1)) * [`8c9e24778`](npm/cli@8c9e247) [#3055](npm/cli#3055) feat(version): add workspace support ([@wraithgar](https://github.com/wraithgar)) ### DEPENDENCIES * [`f1e6743a6`](npm/cli@f1e6743) `libnpmversion@1.2.0` * feat(retrieve-tag): retrieve unannotated git tags * fix(retrieve-tag): use semver to look for semver * [`3b476a24c`](npm/cli@3b476a2) `@npmcl/git@2.0.8` * fix(git): do not use shell when calling git * [`dfcd0c1e2`](npm/cli@dfcd0c1) [#3069](npm/cli#3069) `tap@15.0.2` ### DOCUMENTATION * [`90b61eda9`](npm/cli@90b61ed) [#3053](npm/cli#3053) fix(contributing.md): explicitely outline dep updates ([@darcyclarke](https://github.com/darcyclarke)) ## v7.9.0 (2021-04-08) ### FEATURES * [`1f3e88eba`](npm/cli@1f3e88e) [#3032](npm/cli#3032) feat(dist-tag): add workspace support ([@nlf](https://github.com/nlf)) * [`6e31df4e7`](npm/cli@6e31df4) [#3033](npm/cli#3033) feat(pack): add workspace support ([@wraithgar](https://github.com/wraithgar)) ### DEPENDENCIES * [`ba4f7fea8`](npm/cli@ba4f7fe) `licensee@8.2.0` ## v7.8.0 (2021-04-01) ### FEATURES * [`8bcc5d73f`](npm/cli@8bcc5d7) [#2972](npm/cli#2972) feat(workspaces): add repo and docs ([@wraithgar](https://github.com/wraithgar)) * [`ec520ce32`](npm/cli@ec520ce) [#2998](npm/cli#2998) feat(set-script): implement workspaces * [`32717a60e`](npm/cli@32717a6) [#3001](npm/cli#3001) feat(view): add workspace support ([@wraithgar](https://github.com/wraithgar)) * [`7b177e43f`](npm/cli@7b177e4) [#3014](npm/cli#3014) feat(config): add 'envExport' flag ([@isaacs](https://github.com/isaacs)) ### BUG FIXES * [`4c4252348`](npm/cli@4c42523) [#3016](npm/cli#3016) fix(usage): specify the key each time for multiples ([@isaacs](https://github.com/isaacs)) * [`9237d375b`](npm/cli@9237d37) [#3013](npm/cli#3013) fix(docs): add workspaces configuration ([@wraithgar](https://github.com/wraithgar)) * [`cb6eb0d20`](npm/cli@cb6eb0d) [#3015](npm/cli#3015) fix(ERESOLVE): better errors when current is missing ([@isaacs](https://github.com/isaacs)) ### DEPENDENCIES * [`61da39beb`](npm/cli@61da39b) `@npmcli/config@2.1.0` * feat(config): add support for envExport:false * [`fb095a708`](npm/cli@fb095a7) `@npmcli/arborist@2.3.0`: * [#2896](npm/cli#2896) Provide currentEdge in ERESOLVE if known, and address self-linking edge case. * Add/remove dependencies to/from workspaces when set, not root project * Only reify the portions of the dependency graph identified by the `workspace` configuration value. * Do not recursively `chown` the project root path. ## v7.7.6 (2021-03-29) ### BUG FIXES * [`9dd2ed518`](npm/cli@9dd2ed5) fix empty newline printed to stderr ([@ruyadorno](https://github.com/ruyadorno)) * [`9d391462a`](npm/cli@9d39146) [#2973](npm/cli#2973) fix spelling in workspaces.md file ([@sethomas](https://github.com/sethomas)) * [`4b100249a`](npm/cli@4b10024) [#2979](npm/cli#2979) change 'maxsockets' default value back to 15 ([@wallrat](https://github.com/wallrat)) ### DEPENDENCIES * [`a28f89572`](npm/cli@a28f895) `libnpmversion@1.1.0` * fix reading `script-shell` config on `npm version` lifecycle scripts * [`03734c29e`](npm/cli@03734c2) `npm-packlist@2.1.5` * fix packaging `bundledDependencies` * [`80ce2a019`](npm/cli@80ce2a0) `@npmcli/metavuln-calculator@1.1.1` * fix error auditing package documents with missing dependencies ## v7.7.5 (2021-03-25) ### BUG FIXES * [`95ba87622`](npm/cli@95ba876) [#2949](npm/cli#2949) fix handling manual indexes in `npm help` ([@dmchurch](https://github.com/dmchurch)) * [`59cf37962`](npm/cli@59cf379) [#2958](npm/cli#2958) always set `npm.command` to canonical command name ([@isaacs](https://github.com/isaacs)) * [`1415b4bde`](npm/cli@1415b4b) [#2964](npm/cli#2964) fix(config): properly translate user-agent ([@wraithgar](https://github.com/wraithgar)) * [`59271936d`](npm/cli@5927193) [#2965](npm/cli#2965) fix(config): tie save-exact/save-prefix together ([@wraithgar](https://github.com/wraithgar)) ### TESTS * [`97b415287`](npm/cli@97b4152) [#2959](npm/cli#2959) add smoke tests ([@ruyadorno](https://github.com/ruyadorno))

driskell · 2021-04-24T08:44:28Z

@jurre Was the approach you suggested merged as the PR is showing as closed #3009?

I am now having issues in all my repositories where security updates are no longer appearing because dependabot simply can’t scan the repository in time anymore even though I’ve effectively set it to only check 3 packages. Investigating the logs from GitHub I can see it is downloading the ZIP for many packages each time and never gets to complete the second package... I’m sure it didn’t use to do this but perhaps the updates going in affect more than just the 3 I requested I guess. I think it takes now 5 days or more to even get the updates for the 3 packages which isn’t sustainable.

I also worry how much CO2 this is burning lol.

If there was an issue with the approach in #3009 it be good to know as I’m keen to help get something working. The optimisation PRs at composer that are needed for extremely quick scans with little CO2 are stuck or might need some test attention (composer/composer#9620 composer/composer#9619 composer/composer#9261) but hopefully moving along as the maintainers find time but this issue here looks to me like a good win!

Thanks!

jurre · 2021-04-26T09:50:36Z

Was the approach you suggested merged as the PR is showing as closed

No, I closed it as I ran into a bunch of different test failures that I didn't really have time to investigate. The logs for those have since been garbage collected by Actions, so I've re-opened the PR temporarily to regenerate the CI failures, but I will close the PR again after that, as I'm trying to cut down pending PRs that I'm not actively working on to keep myself sane.

I'm surprised things have gotten that much worse, as nothing has really changed on our end 🤔

It would be great if we could leverage changes to composer like the PR you linked to instead of building something bespoke for Dependabot, but if we can get the config flag working that seems reasonable to me.

mpdude · 2021-05-03T13:19:15Z

Trying to migrate from dependabot-preview to dependabot for a whole bunch of PHP projects, I noticed that updates currently fail for me with the message

Dependabot can't access your-org/your-repo
You can fix the issue by granting Dependabot access, which will enable any repository in @your-org to get automatic updates from your-org/your-repo. Be sure that you want to share your-org/your-repo with everyone in your organization.

I am using a private package registry, but that seems to work find and access is granted (I configured it accordingly). Now, to my knowledge, Composer obtains all the information it needs from the registries, so I wonder why the above error occurs.

Am I right assuming that if the change suggested here were made, it would no longer be necessary to configure the additional permissions in https://github.com/organizations/my-org/settings/security_analysis, under "Grant Dependabot access to private repositories"?

jurre · 2021-05-03T13:59:16Z

@mpdude that seems like a different issue, it appears that you're pulling in a git dependency and Dependabot cannot access it.

I don't think that these changes would affect access to private sources, Dependabot would still need to access them to find version info etc.

Could you open up a new issue for this and fill out the questions in the template? I'll be more than happy to help you sort it out 👍

mpdude · 2021-05-03T20:12:21Z

@jurre Tried my very best over at #3628.

markdorison · 2021-07-27T19:21:35Z

Is this PR still being considered? We are experiencing the issues described in #2416 & #2999, and while there are also some issues/PRs against Composer (composer/composer#9261, composer/composer#9620), I have not seen any recent traction on those. I know the sentiment seemed to be that if it could be fixed in Composer that would be ideal, but if this could be resolved here with little downside, I would be the first to cheer that on.

driskell · 2022-01-14T18:53:05Z

Was the approach you suggested merged as the PR is showing as closed

No, I closed it as I ran into a bunch of different test failures that I didn't really have time to investigate. The logs for those have since been garbage collected by Actions, so I've re-opened the PR temporarily to regenerate the CI failures, but I will close the PR again after that, as I'm trying to cut down pending PRs that I'm not actively working on to keep myself sane.

I'm surprised things have gotten that much worse, as nothing has really changed on our end 🤔

It would be great if we could leverage changes to composer like the PR you linked to instead of building something bespoke for Dependabot, but if we can get the config flag working that seems reasonable to me.

I just tested the setInstall locally to take a look. Using setInstall(false) disables installation completely - it won't even hit the InstallationManager. So the get_latest_resolvable_version request and likely others will never find the packages as it relies on the InstallationManager to return the list of installed packages. I can think also this won't install any installer packages that might impact the output file.

…son/lock files

driskell · 2022-01-14T19:25:08Z

I fixed up the PHPCS for this and used NoopInstaller instead. I also tried to fix up tests but it warrants a bit of discussion - I've mostly repurposed tests for reporting nice errors into tests to ensure no error happens since those tests begin to fail because no attempt is made to download or install and so bad commits and references and variables aren't relevant.

…ked repository to get installed packages

driskell · 2022-01-14T19:48:42Z

Custom installer can change the location of the the installed package on disk. But they cannot change the version being selected (and so cannot affect the lock file).

The only plugins that can affect the lock file are the ones which are hooking into the dependency solver itself (using the pre-pool-create event), like symfony/flex for instance. And for that to work during a composer update, the plugin must be installed before running the update (so either as a global plugin, or by running a composer install first to have a vendor folder).

Sorry I just reread this and took a further look into setInstall() - and ended up just removing the entire custom installation manager (DependabotInstallationManager) - since it occurred to me it's not really the ideal way to get Composer's list of installed packages - we can instead get it from the lock data by using the locker to get the locked packages repository and get the packages there. That is not quite "what did we install just now" but it definitely is the "what is installed overall" and should be equal if not richer to what was currently being used (and I believe it is likely the installation manager was receiving an install operation for every package anyway in the dependabot case... so the lists are likely the same)

Looks like all tests still pass!

driskell · 2022-01-14T19:50:16Z

composer/helpers/v2/src/UpdateChecker.php

@@ -86,7 +77,7 @@ public static function getLatestResolvableVersion(array $args): ?string

        $install->run();

-        $installedPackages = $installationManager->getInstalledPackages();
+        $installedPackages = $composer->getLocker()->getLockedRepository(true)->getPackages();


This boolean argument to getLockedRepository is "withDevReqs" to return all packages including dev requirements.

driskell · 2022-01-15T08:24:49Z

composer/spec/dependabot/composer/file_updater/lockfile_updater_spec.rb

@@ -609,12 +574,8 @@
        )
      end

-      it "raises a helpful errors" do
-        expect { updated_lockfile_content }.to raise_error do |error|
-          expect(error).to be_a Dependabot::GitDependencyReferenceNotFound


Do these exceptions potentially need cleaning up? I left as is as it seems it will need a higher discussion as this looks like the scope now increases as there’s potentially areas of code handing nice errors that won’t need handling anymore because installs aren’t attempted

driskell · 2022-01-15T10:39:20Z

@jurre @stof Found an issue when using setInstall(false) with https://github.com/wikimedia/composer-merge-plugin

That plugin adds additional install requirements based on merges of composer.json from other installed packages. Thus I don't think you can not allow it to be installed, and potentially even you cannot disallow installation of libraries as any one could provide a merge...

In my test runs on one repository I have (I can't share) then during updates dependabot would start to remove the additional requirements provided by the merge plugin....

driskell · 2022-01-15T10:55:40Z

I think just doing this in the UpdateChecker would be fine, as long as the Updater still installs. That would still give a speed benefit for where update not needed.

I accidentally force pushed now I can't reopen heh. Will raise separately once tested.

driskell · 2022-01-15T16:52:14Z

New PR #4635

driskell requested a review from a team as a code owner January 17, 2021 12:39

driskell mentioned this pull request Jan 17, 2021

Drupal project dependabot runs are very slow #2999

Open

driskell mentioned this pull request Jan 17, 2021

Composer job run timing out #2416

Open

jurre mentioned this pull request Jan 19, 2021

Configure composer to not install packages and only update lockfile #3009

Closed

Prevent unnecessary downloads of library packages - just update the j…

b82a0fb

…son/lock files

driskell force-pushed the mock-library-installer-prevent-unnecessary-downloads branch from dad051d to b82a0fb Compare January 14, 2022 19:23

Remove custom installation manager, disable all installs, and use loc…

d6e97a0

…ked repository to get installed packages

driskell force-pushed the mock-library-installer-prevent-unnecessary-downloads branch from d9ce72a to d6e97a0 Compare January 14, 2022 19:46

driskell commented Jan 14, 2022

View reviewed changes

driskell commented Jan 15, 2022

View reviewed changes

driskell closed this Jan 15, 2022

driskell mentioned this pull request Jan 15, 2022

Prevent unnecessary composer package downloads #4635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent unnecessary downloads of library packages - just update the json/lock files #2998

Prevent unnecessary downloads of library packages - just update the json/lock files #2998

driskell commented Jan 17, 2021 •

edited

Loading

driskell commented Jan 17, 2021

jurre commented Jan 17, 2021

driskell commented Jan 18, 2021

stof commented Jan 19, 2021

jurre commented Jan 19, 2021

stof commented Jan 19, 2021

driskell commented Jan 19, 2021

stof commented Jan 19, 2021

driskell commented Apr 24, 2021 •

edited

Loading

jurre commented Apr 26, 2021

mpdude commented May 3, 2021

jurre commented May 3, 2021

mpdude commented May 3, 2021

markdorison commented Jul 27, 2021

driskell commented Jan 14, 2022

driskell commented Jan 14, 2022

driskell commented Jan 14, 2022 •

edited

Loading

driskell Jan 14, 2022 •

edited

Loading

driskell Jan 15, 2022

driskell commented Jan 15, 2022

driskell commented Jan 15, 2022

driskell commented Jan 15, 2022

Prevent unnecessary downloads of library packages - just update the json/lock files #2998

Prevent unnecessary downloads of library packages - just update the json/lock files #2998

Conversation

driskell commented Jan 17, 2021 • edited Loading

driskell commented Jan 17, 2021

jurre commented Jan 17, 2021

driskell commented Jan 18, 2021

stof commented Jan 19, 2021

jurre commented Jan 19, 2021

stof commented Jan 19, 2021

driskell commented Jan 19, 2021

stof commented Jan 19, 2021

driskell commented Apr 24, 2021 • edited Loading

jurre commented Apr 26, 2021

mpdude commented May 3, 2021

jurre commented May 3, 2021

mpdude commented May 3, 2021

markdorison commented Jul 27, 2021

driskell commented Jan 14, 2022

driskell commented Jan 14, 2022

driskell commented Jan 14, 2022 • edited Loading

driskell Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

driskell Jan 15, 2022

Choose a reason for hiding this comment

driskell commented Jan 15, 2022

driskell commented Jan 15, 2022

driskell commented Jan 15, 2022

driskell commented Jan 17, 2021 •

edited

Loading

driskell commented Apr 24, 2021 •

edited

Loading

driskell commented Jan 14, 2022 •

edited

Loading

driskell Jan 14, 2022 •

edited

Loading