AtomicFileTransaction rename bin_dir #3

Open
fpoirotte opened this Issue Apr 26, 2011 · 9 comments

Comments

Projects
None yet
6 participants
Member

fpoirotte commented Apr 26, 2011

Copied from http://pear.php.net/bugs/bug.php?id=17627

Description:

When installing PEAR2_Pyrus the following exception occures:

 "PEAR2\Pyrus\AtomicFileTransaction\Exception" with message "Cannot rollback - not in a transaction" in E:\workspace\all\Pyrus\src\Pyrus\AtomicFileTransaction.php:535

This exception originates in AtomicFileTransaction.php line 655.
The reason for the exception is that it tries to rename "C:\xampp.old-php" to "C:\xampp\php" and this is not possible because php.exe is in use.

Maybe related to https://pear.php.net/bugs/bug.php?id=17195

Test script:

php scripts/pyrus e:\test
php scripts/pyrus e:\test set preferred_state alpha
php scripts/pyrus e:\test install PEAR2_Pyrus

Expected result:

  • A successful installation
    or
  • Better exception indicating that the bin_dir needs to be changed.

Actual result:

exception "PEAR2\Pyrus\AtomicFileTransaction\Exception" with message "Cannot rollback - not in a transaction" in E:\workspace\all\Pyrus\src\Pyrus\AtomicFileTransaction.php:535
Stack trace:
0 E:\workspace\all\Pyrus\src\Pyrus\Installer.php(329): PEAR2\Pyrus\AtomicFileTransaction::rollback()
1 E:\workspace\all\Pyrus\src\Pyrus\ScriptFrontend\Commands.php(482): PEAR2\Pyrus\Installer::commit()
2 E:\workspace\all\Pyrus\src\Pyrus\ScriptFrontend\Commands.php(276): PEAR2\Pyrus\ScriptFrontend\Commands->install(Array, Array)
3 E:\workspace\all\Pyrus\scripts\pyrus(37): PEAR2\Pyrus\ScriptFrontend\Commands->run(Array)
4 {main}PEAR2\Pyrus\AtomicFileTransaction\Exception: Cannot rollback - not in a transaction
Owner

saltybeagle commented May 16, 2011

@boekkooi Hey Warnar, I wonder if you have any thoughts on this issue. Should we not handle bin_dir transactions in this way?

Contributor

boekkooi commented May 16, 2011

@saltybeagle What we can do is use a temp directory for the backup and journal version of the atomic file transaction. Then on rollback we can see if the parent directory is writable and then just use the rename or when it is not just copy over each file.

See :
https://github.com/boekkooi/PEAR2_Pyrus/blob/master/src/PEAR2/Pyrus/AtomicFileTransaction/Transaction/Base.php - Line: 140
https://github.com/boekkooi/PEAR2_Pyrus/blob/master/src/PEAR2/Pyrus/AtomicFileTransaction/Transaction/TwoStage.php - Line: 81 & 135 & 142

Also bug probably happens because rollback is called twice.

Do you want to take this bug or shall I?

Owner

saltybeagle commented May 16, 2011

@boekkooi If you could look into them, that would be fantastic. :-)

Contributor

boekkooi commented May 17, 2011

@saltybeagle what do you think about this idea: http://pastebin.com/qFmiQT6q

Owner

saltybeagle commented May 23, 2011

@boekkooi That sounds like a good direction to go.

Contributor

cellog commented Jun 23, 2011

the pastebin expired, would love to see the idea. I LOVE your idea of using a temp directory. I had thought we were doomed to remove atomic file transactions for bin_dir when I first read this.

Owner

saltybeagle commented Jun 23, 2011

diff --git a/src/PEAR2/Pyrus/Filesystem.php b/src/PEAR2/Pyrus/Filesystem.php
index fed2551..ca58155 100644
--- a/src/PEAR2/Pyrus/Filesystem.php
+++ b/src/PEAR2/Pyrus/Filesystem.php
@@ -2,6 +2,7 @@
 namespace PEAR2\Pyrus;

 use FilesystemIterator;
+use Exception as IOException;

 abstract class Filesystem
 {
@@ -108,28 +109,30 @@ abstract class Filesystem
     public static function copyDir($source, $target)
     {
         if (!file_exists($source)) {
-            return;
+            return null;
         }

         try {
-            $done = array();
+            $hashMap = array();
             $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($source),
                                                    \RecursiveIteratorIterator::SELF_FIRST);
             while ($iterator->valid()) {
+                /** @var $iterator \RecursiveDirectoryIterator */
                 if ($iterator->isDot()) {
                     $iterator->next();
                     continue;
                 }

+                /** @var $file \SplFileInfo */
                 $file = $iterator->current();
+                $subPath = $iterator->getSubPathName();
                 $targetPath = $target . DIRECTORY_SEPARATOR . $iterator->getSubPathName();
                 $iterator->next();

                 // It seems RecursiveIteratorIterator can return the save file twice
-                if (isset($done[$targetPath])) {
+                if (isset($hashMap[$subPath])) {
                     continue;
                 }
-                $done[$targetPath] = true;

                 // Copy directory or file
                 if ($file->isDir()) {
@@ -151,9 +154,51 @@ abstract class Filesystem
                     throw new IOException(
                         'Unable to copy directory, touch failed');
                 }
+
+                if ($iterator->isFile()) {
+                    $hashMap[$subPath] = md5_file($file);
+                } else {
+                    $hashMap[$subPath] = $subPath;
+                }
             }
+
+            return $hashMap;
         } catch (\UnexpectedValueException $e) {
             throw new IOException('directory copy failed: ' . $e->getMessage(), $e);
         }
     }
+
+    public static function changes($source, $originalHashMap)
+    {
+        $new = array();
+        $removed = $originalHashMap;
+        $changed = array();
+
+        $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($source),
+            \RecursiveIteratorIterator::SELF_FIRST);
+        $iterator->rewind();
+        while ($iterator->valid()) {
+            /** @var $iterator \RecursiveDirectoryIterator */
+            if ($iterator->isDot()) {
+                $iterator->next();
+                continue;
+            }
+
+            $file = $iterator->current();
+            $subPath = $iterator->getSubPathName();
+
+            if (!isset($originalHashMap[$subPath])) {
+                $new[] = $subPath;
+            } else {
+                if ($iterator->isFile() && md5_file($file) !== $originalHashMap[$subPath]) {
+                    $changed[] = $subPath;
+                }
+                unset($removed[$subPath]);
+            }
+
+            $iterator->next();
+        }
+
+        return array('new' => $new, 'removed' => $removed, 'changed' => $changed);
+    }
 }

Will this ever be fixed? See also #9.

Let's make a new comment to this issue each year. (Last ones are 2013,2012 and 2011)

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