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

occ scan:files Adding more details in the base print out (II) #21701

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

mmattel
Copy link
Contributor

@mmattel mmattel commented Jan 13, 2016

This is a restart of #21445 because of #21504 (Why are some PR checks passing and why are some checks stuck).

This is a PR for adding more details in the base printout of ./occ files:scan

Reference issue #21412 (occ files:scan missing feedback while running)
@jancborchardt @karlitschek

All the other parameters stay the same. When adding eg --verbose, you get the details of each file/directory beeing scanned.

I have implemented the following:

sudo -uwww-data ./occ files:scan user1 -v

Starting scan for user 1 out of 1 (user1)
    File   /user1/
    Folder /user1/
    File   /user1/files
    Folder /user1/files
    File   /user1/files/welcome.txt

+---------+-------+--------------+
| Folders | Files | Elapsed time |
+---------+-------+--------------+
| 2       | 3     | 00:00:00     |
+---------+-------+--------------+
sudo -uwww-data ./occ files:scan --all -v

Scanning files for 3 users

Starting scan for user 1 out of 3 (user0)
    File   /user0/
    Folder /user0/
    File   /user0/files
    File   /user0/thumbnails
    File   /user0/cache
    Folder /user0/files
    File   /user0/files/welcome.txt
    File   /user0/files/folder2
    File   /user0/files/folder1
    Folder /user0/files/folder2
    Folder /user0/files/folder1
    File   /user0/files/folder1/hs_err_pid3522.log
    File   /user0/files/folder1/x11vnc-0.9.14-dev.tar.gz
    Folder /user0/thumbnails
    File   /user0/thumbnails/3
    Folder /user0/thumbnails/3
    File   /user0/thumbnails/3/2048-2048-max.png
    File   /user0/thumbnails/3/32-32.png
    Folder /user0/cache

Starting scan for user 2 out of 3 (user1)
    File   /user1/
    Folder /user1/
    File   /user1/files
    Folder /user1/files
    File   /user1/files/welcome.txt

Starting scan for user 3 out of 3 (user2)
    File   /user2/
    Folder /user2/
    File   /user2/files
    Folder /user2/files
    File   /user2/files/welcome.txt

+---------+-------+--------------+
| Folders | Files | Elapsed time |
+---------+-------+--------------+
| 11      | 18    | 00:00:00     |
+---------+-------+--------------+

sudo -uwww-data ./occ files:scan --all 

Scanning files for 3 users
Starting scan for user 1 out of 3 (user0)
Starting scan for user 2 out of 3 (user1)
Starting scan for user 3 out of 3 (user2)

+---------+-------+--------------+
| Folders | Files | Elapsed time |
+---------+-------+--------------+
| 11      | 18    | 00:00:00     |
+---------+-------+--------------+

There was a discussion in the original PR having the username in the middle or not.
I decided to stay with the username at the end because it then will be formatted properly.
Please see the examples below:

Scanning files of mm (1 of 3 users)
Scanning files of verylogusername (2 of 3 users)
Scanning files of user0 (3 of 3 users)

Starting scan for user 1 out of 3 (mm)
Starting scan for user 2 out of 3 (verylogusername)
Starting scan for user 3 out of 3 (user0)

The table printout at the end is a symfony function which renders automatically. Pls see http://symfony.com/doc/current/components/console/helpers/table.html

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @PVince81, @butonic and @icewind1991 to be potential reviewers

@icewind1991
Copy link
Contributor

👍

@DeepDiver1975 DeepDiver1975 added this to the 9.0-current milestone Jan 15, 2016
@DeepDiver1975
Copy link
Member

👍

@DeepDiver1975
Copy link
Member

NOTE: the whole interruption logic doesn't seem to work for me ....

@mmattel
Copy link
Contributor Author

mmattel commented Jan 16, 2016

@DeepDiver1975 fixed the interruption logic and squashed the commits

@mmattel
Copy link
Contributor Author

mmattel commented Jan 20, 2016

@DeepDiver1975 would you mind to give this a try again and based on success merge it?

@PVince81
Copy link
Contributor

Doesn't work for me:

± % sudo -u wwwrun ./occ files:scan --all                                                                                                  !10380
PHP Fatal error:  Call to undefined function OCA\Files\Command\pcntl_signal() in /srv/www/htdocs/owncloud/apps/files/command/scan.php on line 226
PHP Stack trace:
PHP   1. {main}() /srv/www/htdocs/owncloud/occ:0
PHP   2. require_once() /srv/www/htdocs/owncloud/occ:11
PHP   3. OC\Console\Application->run($input = *uninitialized*, $output = *uninitialized*) /srv/www/htdocs/owncloud/console.php:81
PHP   4. Symfony\Component\Console\Application->run($input = *uninitialized*, $output = *uninitialized*) /srv/www/htdocs/owncloud/lib/private/console/application.php:110
PHP   5. Symfony\Component\Console\Application->doRun($input = *uninitialized*, $output = *uninitialized*) /srv/www/htdocs/owncloud/3rdparty/symfony/console/Application.php:123
PHP   6. Symfony\Component\Console\Application->doRunCommand($command = *uninitialized*, $input = *uninitialized*, $output = *uninitialized*) /srv/www/htdocs/owncloud/3rdparty/symfony/console/Application.php:192
PHP   7. Symfony\Component\Console\Command\Command->run($input = *uninitialized*, $output = *uninitialized*) /srv/www/htdocs/owncloud/3rdparty/symfony/console/Application.php:841
PHP   8. OCA\Files\Command\Scan->execute($input = *uninitialized*, $output = *uninitialized*) /srv/www/htdocs/owncloud/3rdparty/symfony/console/Command/Command.php:259
PHP   9. OCA\Files\Command\Scan->initTools() /srv/www/htdocs/owncloud/apps/files/command/scan.php:184

@DeepDiver1975
Copy link
Member

pcntl_signal

@mmattel you have to check if pcntl is available - http://php.net/manual/en/pcntl.installation.php

@mmattel mmattel force-pushed the scan_files_more_details_II branch from cd24ab3 to a9a84b4 Compare January 22, 2016 17:30
@mmattel
Copy link
Contributor Author

mmattel commented Jan 22, 2016

@DeepDiver1975 thanks for bringing this up. I am running Ubuntu and I did not had problems with and never thought that this could be a problem. When I looked in http://php.net/manual/de/function.pcntl-signal.php I saw that it is "present" in our supported php version. But it seems that it is system dependent for beeing available or not (your link mentions that)...
I now added a check if the pcntl_signal function is available and if not, the process can´t be interrupted with ctrl-c anymore - it just runs thru.

@PVince81 you are running openSUSE, could that be that this OS (or others) need special treatement to enable pcntl_signal function calls for php?

@PVince81 @DeepDiver1975 how can we check that or describe in the documentation the need for php pcntl_signal functions? Maybe a info in the admin page at the top? That would at least give us a hint about possible system setup needs.

@mmattel
Copy link
Contributor Author

mmattel commented Jan 25, 2016

@PVince81 May I ask you a favour:

  • Can you check if you get success when you run php -m | grep pcntl
    (from my understanding means that it is available/compiled in when you get pcntl as answer)
  • And does your php5/cli/php.ini (this is the path for Ubuntu) has pcntl_xxx functions disabled in the directive: disable_functions =

I am trying to find a good way identifying if pcntl functions are available on the hosting system.

@PVince81
Copy link
Contributor

@mmattel

  • php -m | grep pcntl gives no results
  • no functions are disabled

I'm running openSUSE Tumbleweed

@mmattel
Copy link
Contributor Author

mmattel commented Jan 25, 2016

@PVince81 Thanks, I like this answer 😄 ! because it means we can check against and give advices to enable it.

  • My system is on Ubuntu 14.04 which has pcntl already compiled in and shipped with php.
  • You can always disable a existing php module for any reason you might have with the disable_functions directive in any appropriate php.ini
  • Some systems like yours need to get the php module seperately, pls see as example http://rpmfind.net/linux/rpm2html/search.php?query=php5-pcntl

With the current code of this PR, the existance of the pcntl_system function is checked upfront and if not present, no error will be raised, the code can not be interrupted by ctrl-c. If present, you get the ability to clean interrupt a possible long running occ command. I have already informed @oparoz as he has also written a PR for a occ gallery command having the ability for user interruption.

My next steps would be to update the documentation with regards to the pcntl php module and to create a PR to show at the top of the admin page a note if the pcntl php module is missing (may take some time due to regular business...).

Would you mind to test this PR again?
In your current configuration, the code should not create an error and can´t be interrupted.

@@ -110,9 +120,15 @@ protected function scanFiles($user, $path, $verbose, OutputInterface $output) {
} else {
$scanner->listen('\OC\Files\Utils\Scanner', 'scanFile', function ($path) use ($output) {
$this->filesCounter += 1;
if ($this->hasBeenInterrupted()) {
throw new \Exception('crtl-c');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> ctrl, not crtl

@PVince81
Copy link
Contributor

Apart from the copy-pasted typo, seems to work fine 👍 when the PHP module for interruption is not present

@mmattel mmattel force-pushed the scan_files_more_details_II branch from a9a84b4 to ee8d470 Compare January 26, 2016 17:16
@mmattel
Copy link
Contributor Author

mmattel commented Jan 26, 2016

Fixed the text typo (crtl-c --> ctrl-c)

@mmattel mmattel force-pushed the scan_files_more_details_II branch from ee8d470 to c3122ac Compare January 26, 2016 17:42
@mmattel
Copy link
Contributor Author

mmattel commented Jan 27, 2016

@DeepDiver1975 after the ok from @PVince81, merge?

@PVince81
Copy link
Contributor

@icewind1991 @DeepDiver1975 ready for merge ? (needs rereview for pnctl fallback)

Use proper method name

Fixed the interruption logic

Checks the availability of the pcntl_signal function

Fixed typo crtl-c --> ctrl-c

one overseen crtl-c typo
@rullzer rullzer force-pushed the scan_files_more_details_II branch from c3122ac to e05592d Compare January 29, 2016 07:42
@rullzer
Copy link
Contributor

rullzer commented Jan 29, 2016

Had to rebase since the files app has had a version bump.
But now everything is working fine over here with and without pnctl.

👍

DeepDiver1975 added a commit that referenced this pull request Jan 29, 2016
occ scan:files Adding more details in the base print out (II)
@DeepDiver1975 DeepDiver1975 merged commit b65a23c into master Jan 29, 2016
@DeepDiver1975 DeepDiver1975 deleted the scan_files_more_details_II branch January 29, 2016 10:41
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants