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

Allow to run a satis build for one repo/package #125

Merged
merged 8 commits into from May 1, 2014

Conversation

Projects
None yet
@shochdoerfer
Contributor

shochdoerfer commented Feb 25, 2014

Added an additional flag to generate the satis build for a single package. This helps to speed up the satis run if you have to deal with a lot of repos. In our case Jenkins will trigger the satis build after a new release was created. Since we have a lot packages (with a lot of versions) in our local satis repo, I was looking for a way to speed up the satis run.

Added additional flag to generate the satis build for a single packag…
…e. This helps to speed up the satis run if you have a lot of repos.
@stof

View changes

Show outdated Hide outdated src/Composer/Satis/Command/BuildCommand.php
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 25, 2014

Contributor

your filtered package seems to be missing the exclusion of AliasPackage and stability

Contributor

stof commented Feb 25, 2014

your filtered package seems to be missing the exclusion of AliasPackage and stability

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Feb 25, 2014

Contributor

@stof thanks for pointing out. Will add that as well.

Contributor

shochdoerfer commented Feb 25, 2014

@stof thanks for pointing out. Will add that as well.

shochdoerfer added some commits Feb 25, 2014

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Mar 18, 2014

Contributor

When can I expect the PR to be merged in? Or is some more work needed on my side?

Contributor

shochdoerfer commented Mar 18, 2014

When can I expect the PR to be merged in? Or is some more work needed on my side?

@adamlc

This comment has been minimized.

Show comment
Hide comment
@adamlc

adamlc Mar 24, 2014

👍 +1 on this.

We only really want Satis to update the individual projects that have passed the tests on our CI server.

adamlc commented Mar 24, 2014

👍 +1 on this.

We only really want Satis to update the individual projects that have passed the tests on our CI server.

@akimsko

This comment has been minimized.

Show comment
Hide comment
@akimsko

akimsko Mar 28, 2014

This would be a much welcomed feature. Anything in the way of merging this?

akimsko commented Mar 28, 2014

This would be a much welcomed feature. Anything in the way of merging this?

@auipga

This comment has been minimized.

Show comment
Hide comment
@auipga

auipga Apr 16, 2014

Contributor

👍 +2 on this.

Contributor

auipga commented Apr 16, 2014

👍 +2 on this.

@jakeasmith

This comment has been minimized.

Show comment
Hide comment
@jakeasmith

jakeasmith Apr 16, 2014

👍 this would be fantastic

jakeasmith commented Apr 16, 2014

👍 this would be fantastic

@ralouphie

This comment has been minimized.

Show comment
Hide comment
@ralouphie

ralouphie commented Apr 16, 2014

+1!

@Burgov

This comment has been minimized.

Show comment
Hide comment
@Burgov

Burgov Apr 18, 2014

Big 👍!

Burgov commented Apr 18, 2014

Big 👍!

@kmelia

This comment has been minimized.

Show comment
Hide comment
@kmelia

kmelia Apr 22, 2014

I actually deal with more than 200 repositories and this will be helpfull.

But I see that the most time past during the build is for building packages.json (if no newer package are found) Did the pull request fix that ?

kmelia commented Apr 22, 2014

I actually deal with more than 200 repositories and this will be helpfull.

But I see that the most time past during the build is for building packages.json (if no newer package are found) Did the pull request fix that ?

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Apr 22, 2014

Contributor

@kmelia That is a good question. Honestly I do not know. We only deal with 10ish repos. Would you mind testing it in your setup?

Contributor

shochdoerfer commented Apr 22, 2014

@kmelia That is a good question. Honestly I do not know. We only deal with 10ish repos. Would you mind testing it in your setup?

@kmelia

This comment has been minimized.

Show comment
Hide comment
@kmelia

kmelia Apr 22, 2014

I patch my file vendor/composer/satis/src/Composer/Satis/Command/BuildCommand.php and I have run with just one package : but my packages.json did not keep the others packages. Is that normal ?

kmelia commented Apr 22, 2014

I patch my file vendor/composer/satis/src/Composer/Satis/Command/BuildCommand.php and I have run with just one package : but my packages.json did not keep the others packages. Is that normal ?

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Apr 24, 2014

Member

Indeed just looking at the code of this it would generate a packages.json only for the filtered packages, not update an existing one with the filtered package info?

Member

naderman commented Apr 24, 2014

Indeed just looking at the code of this it would generate a packages.json only for the filtered packages, not update an existing one with the filtered package info?

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Apr 24, 2014

Contributor

@kmelia @naderman Thanks for pointing out. Seems I misinterpreted the results. Will work on the update part in the next couple of days.

Contributor

shochdoerfer commented Apr 24, 2014

@kmelia @naderman Thanks for pointing out. Seems I misinterpreted the results. Will work on the update part in the next couple of days.

@akimsko

This comment has been minimized.

Show comment
Hide comment
@akimsko

akimsko Apr 24, 2014

@shochdoerfer I added a PR to your fork/branch that should solve this, and some other issues.
https://github.com/bitExpert/satis/pull/1

akimsko commented Apr 24, 2014

@shochdoerfer I added a PR to your fork/branch that should solve this, and some other issues.
https://github.com/bitExpert/satis/pull/1

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Apr 26, 2014

Contributor

Thanks to @akimsko the packages.json file gets now generated correctly as well as the html file. @kmelia Would you mind testing the patch in your setup?

Contributor

shochdoerfer commented Apr 26, 2014

Thanks to @akimsko the packages.json file gets now generated correctly as well as the html file. @kmelia Would you mind testing the patch in your setup?

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Apr 26, 2014

Member

Unfortunately I recently merged a change to use includes in packages.json so I think this doesn't quite work correctly with that. Can you rebase onto latest master and adapt this branch?

Member

naderman commented Apr 26, 2014

Unfortunately I recently merged a change to use includes in packages.json so I think this doesn't quite work correctly with that. Can you rebase onto latest master and adapt this branch?

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Apr 26, 2014

Contributor

@naderman I merged in the changes from master this morning and changed the code to make it work with it.

Contributor

shochdoerfer commented Apr 26, 2014

@naderman I merged in the changes from master this morning and changed the code to make it work with it.

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Apr 26, 2014

Member

Oh sorry, I missed that :)

Member

naderman commented Apr 26, 2014

Oh sorry, I missed that :)

naderman added a commit that referenced this pull request May 1, 2014

Merge pull request #125 from bitExpert/feature/single_package_build
Allow to run a satis build for one repo/package

@naderman naderman merged commit 11c9b28 into composer:master May 1, 2014

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Jul 10, 2014

Contributor

I've been playing with this and was hoping that a such a single-package build would only fetch and re-scan the vcs repository for the given package.

In fact, the console output shows that satis iterates over all repositories again.

Am I missing anything or did I get this feature wrong?

Contributor

mpdude commented Jul 10, 2014

I've been playing with this and was hoping that a such a single-package build would only fetch and re-scan the vcs repository for the given package.

In fact, the console output shows that satis iterates over all repositories again.

Am I missing anything or did I get this feature wrong?

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Jul 10, 2014

Contributor

BuildCommand::selectPackages() will as a first step do

         $repos = $composer->getRepositoryManager()->getRepositories();
          $pool = new Pool($minimumStability);
          foreach ($repos as $repo) {
              try {
                  $pool->addRepository($repo);
              } ...
          }

... which will pull and re-scan all defined VcsRepositories even before the filter is applied.

Contributor

mpdude commented Jul 10, 2014

BuildCommand::selectPackages() will as a first step do

         $repos = $composer->getRepositoryManager()->getRepositories();
          $pool = new Pool($minimumStability);
          foreach ($repos as $repo) {
              try {
                  $pool->addRepository($repo);
              } ...
          }

... which will pull and re-scan all defined VcsRepositories even before the filter is applied.

@akimsko

This comment has been minimized.

Show comment
Hide comment
@akimsko

akimsko Jul 10, 2014

Well no, you didnt get it wrong. But it kinda only solved half the problem - the actual package fetching.

TMK you can't select a repo based on package. The information just isn't available. You can only ask if the repo contains a specific package, wich potentially scans all repos to find one specific package.

I guess you could store which repo the package came from (in packages.json or somewhere else), and read from that. But in theory a package can change repo (and maybe even be split across multiple repos in different versions?). So a full scan is the only way to be sure you get all versions of the package.

Of course it could fall back to a full scan, if it can no longer find the package in the last repo it got it from. That would be fairly safe, and very rarely result in a full scan. But it would possibly create some edge cases where new versions wouldn't be found, for instance if the package moved repo, but didnt get removed from the old one.

akimsko commented Jul 10, 2014

Well no, you didnt get it wrong. But it kinda only solved half the problem - the actual package fetching.

TMK you can't select a repo based on package. The information just isn't available. You can only ask if the repo contains a specific package, wich potentially scans all repos to find one specific package.

I guess you could store which repo the package came from (in packages.json or somewhere else), and read from that. But in theory a package can change repo (and maybe even be split across multiple repos in different versions?). So a full scan is the only way to be sure you get all versions of the package.

Of course it could fall back to a full scan, if it can no longer find the package in the last repo it got it from. That would be fairly safe, and very rarely result in a full scan. But it would possibly create some edge cases where new versions wouldn't be found, for instance if the package moved repo, but didnt get removed from the old one.

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Jul 10, 2014

Contributor

Ok, trying to get this straight...

This PR allows to update selectively by providing a package name. It will then re-scan all configured repositories, figure out which versions of the package(s) are available in all of them and then dumpDownload() only versions of this package.

In contrary, PR #110 together with jkufner/satis#1 takes a repo URL as parameter and only re-scans this given repo for available packages. It then merges that information with the full set of packages which is kept serialized in a cache.

What I am aiming for are quick updates by mean of a webhook, as in #40. The URL-based "scan only one repo"-approach seems more fit here.

Contributor

mpdude commented Jul 10, 2014

Ok, trying to get this straight...

This PR allows to update selectively by providing a package name. It will then re-scan all configured repositories, figure out which versions of the package(s) are available in all of them and then dumpDownload() only versions of this package.

In contrary, PR #110 together with jkufner/satis#1 takes a repo URL as parameter and only re-scans this given repo for available packages. It then merges that information with the full set of packages which is kept serialized in a cache.

What I am aiming for are quick updates by mean of a webhook, as in #40. The URL-based "scan only one repo"-approach seems more fit here.

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Jul 10, 2014

Contributor

Looking at the code more closely:

  • The new $packagesFilter argument in selectPackages serves to limit the list of packages available for dumping (line 154).
  • Immediately afterwards (line 159ff) we need to "heal" this filtered package list by re-loading dumped package information.
  • From that point on, nothing differs in behaviour.

So - wouldn't it be easier to apply the filtering in dumpDownloads only? That would make a much simpler implementation of the same behaviour IMO.

Contributor

mpdude commented Jul 10, 2014

Looking at the code more closely:

  • The new $packagesFilter argument in selectPackages serves to limit the list of packages available for dumping (line 154).
  • Immediately afterwards (line 159ff) we need to "heal" this filtered package list by re-loading dumped package information.
  • From that point on, nothing differs in behaviour.

So - wouldn't it be easier to apply the filtering in dumpDownloads only? That would make a much simpler implementation of the same behaviour IMO.

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Nov 21, 2014

Contributor

@shochdoerfer could you please try to explain how this filter is supposed to work together with the require and require-all configuration settings, require-depenencies and require-dev-dependencies?

Thanks!

Contributor

mpdude commented Nov 21, 2014

@shochdoerfer could you please try to explain how this filter is supposed to work together with the require and require-all configuration settings, require-depenencies and require-dev-dependencies?

Thanks!

@shochdoerfer

This comment has been minimized.

Show comment
Hide comment
@shochdoerfer

shochdoerfer Nov 24, 2014

Contributor

@mpdude is my comment in #184 enough? Should the discussion continue there?

Contributor

shochdoerfer commented Nov 24, 2014

@mpdude is my comment in #184 enough? Should the discussion continue there?

@shochdoerfer shochdoerfer deleted the bitExpert:feature/single_package_build branch Jan 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment