Francois Gouget : testbot/WinePatchesWebSubmit: Improve robustness, particularly when the Engine is not running.

[Alexandre Julliard]

Module: tools
Branch: master
Commit: 82049e35353b4da6d074977cf1b05003c22283dd
URL:    http://source.winehq.org/git/tools.git/?a=commit;h=82049e35353b4da6d074977cf1b05003c22283dd

Author: Francois Gouget <fgouget at codeweavers.com>
Date:   Wed May  7 16:43:47 2014 +0200

testbot/WinePatchesWebSubmit: Improve robustness, particularly when the Engine is not running.

Now the Engine will automatically pick up patches from the patches
website as part of its periodic 'SafetyNet' check.  However
WinePatchesWebSubmit can still be used to avoid the potential 10
minutes delay.  Also, running WinePatchesWebSubmit when the Engine is
stopped no longer has the side effect of leaking files in the staging
directory.

---

 testbot/bin/Engine.pl                    |   86 +++++++++++++++++++-----------
 testbot/bin/WinePatchesWebSubmit.pl      |   53 +++---------------
 testbot/lib/WineTestBot/Engine/Notify.pm |    4 +-
 3 files changed, 63 insertions(+), 80 deletions(-)

diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl
index 2bb563a..72e165c 100755
--- a/testbot/bin/Engine.pl
+++ b/testbot/bin/Engine.pl
@@ -497,48 +497,73 @@ sub HandleWinePatchMLSubmission
 
 sub HandleWinePatchWebSubmission
 {
-  # Validate file name
-  if ($_[0] !~ m/([0-9a-fA-F]{32}_patch_\d+)/)
+  my $LatestWebPatchId = 0;
+  my $Patches = CreatePatches();
+  foreach my $Patch (@{$Patches->GetItems()})
   {
-    return "0Invalid file name";
+    my $WebPatchId = $Patch->WebPatchId;
+    if (defined $WebPatchId and $LatestWebPatchId < $WebPatchId)
+    {
+      $LatestWebPatchId = $WebPatchId;
+    }
   }
-  my $FullFileName = "$DataDir/staging/$1";
-  if ($_[1] !~ m/^(\d+)$/)
+
+  # Rescan the directory for robustness in case the patches site has already
+  # expired $LatestWebPatchId+1 (for instance if the WineTestBot has not been
+  # run for a while).
+  my (@ErrMessages, @WebPatchIds);
+  if (opendir(my $dh, "$DataDir/webpatches"))
   {
-    return "0Invalid patch id";
+    foreach my $Entry (readdir($dh))
+    {
+      next if ($Entry !~ /^(\d+)$/);
+      my $WebPatchId = $1;
+      next if ($WebPatchId <= $LatestWebPatchId);
+
+      my $Patches = CreatePatches($Patches);
+      $Patches->AddFilter("WebPatchId", [$WebPatchId]);
+      if (@{$Patches->GetKeys()})
+      {
+        push @ErrMessages, "$WebPatchId already exists and yet the latest patch is $LatestWebPatchId";
+        next;
+      }
+      push @WebPatchIds, $WebPatchId;
+    }
+    close($dh);
   }
-  my $WebPatchId = $1;
-  my $Patches = CreatePatches();
-  $Patches->AddFilter("WebPatchId", [$WebPatchId]);
-  if (@{$Patches->GetKeys()})
+  else
   {
-    LogMsg "Patch $WebPatchId already exists\n";
-    return "1OK";
+    return "0Unable to open '$DataDir/webpatches': $!";
   }
 
-  # Create a working dir
-  my $WorkDir = "$DataDir/staging/" . GenerateRandomString(32) . "_work";
-  while (-e $WorkDir)
+  # Add the patches in increasing WebPatchId order so that next time
+  # $LatestWebPatchId still makes sense in case something goes wrong now
+  foreach my $WebPatchId (sort { $a <=> $b } @WebPatchIds)
   {
-    $WorkDir = "$DataDir/staging/" . GenerateRandomString(32) . "_work";
-  }
-  mkdir $WorkDir;
+    # Create a working dir
+    my $WorkDir = "$DataDir/staging/" . GenerateRandomString(32) . "_work";
+    while (-e $WorkDir)
+    {
+      $WorkDir = "$DataDir/staging/" . GenerateRandomString(32) . "_work";
+    }
+    mkdir $WorkDir;
 
-  # Process the patch
-  my $Parser = new MIME::Parser;
-  $Parser->output_dir($WorkDir);
-  my $Entity = $Parser->parse_open($FullFileName);
-  my $ErrMessage = CreatePatches()->NewPatch($Entity, $WebPatchId);
+    # Process the patch
+    my $Parser = new MIME::Parser;
+    $Parser->output_dir($WorkDir);
+    my $Entity = $Parser->parse_open("$DataDir/webpatches/$WebPatchId");
+    my $ErrMessage = $Patches->NewPatch($Entity, $WebPatchId);
+    push @ErrMessages, $ErrMessage if (defined $ErrMessage);
 
-  # Clean up
-  if (!rmtree($WorkDir))
-  {
-    # Not a fatal error but log it to help diagnosis
-    LogMsg "Unable to delete '$WorkDir': $!\n";
+    # Clean up
+    if (!rmtree($WorkDir))
+    {
+      # Not a fatal error but log it to help diagnosis
+      LogMsg "Unable to delete '$WorkDir': $!\n";
+    }
   }
-  unlink($FullFileName);
 
-  return defined($ErrMessage) ? "0" . $ErrMessage : "1OK";
+  return @ErrMessages ? "0". join("; ", @ErrMessages) : "1OK";
 }
 
 sub HandleGetScreenshot
@@ -629,6 +654,7 @@ sub SafetyNet
 {
   CheckJobs();
   ScheduleJobs();
+  HandleWinePatchWebSubmission();
 
   my $Set = WineTestBot::PendingPatchSets::CreatePendingPatchSets();
   my $ErrMessage = $Set->CheckForCompleteSet();
diff --git a/testbot/bin/WinePatchesWebSubmit.pl b/testbot/bin/WinePatchesWebSubmit.pl
index d5b032a..15a5fd5 100755
--- a/testbot/bin/WinePatchesWebSubmit.pl
+++ b/testbot/bin/WinePatchesWebSubmit.pl
@@ -37,56 +37,15 @@ sub BEGIN
   }
 }
 
-use File::Copy;
-use WineTestBot::Config;
 use WineTestBot::Log;
-use WineTestBot::Utils;
-use WineTestBot::Patches;
 use WineTestBot::Engine::Notify;
 
-my ($MaxCount) = @ARGV;
-if (defined $MaxCount)
+# Just tell the Engine to rescan the webpatches directory
+my $ErrMessage = WinePatchWebSubmission();
+if (defined $ErrMessage)
 {
-  $MaxCount =~ m/^(\d+)$/;
-  $MaxCount = $1;
+  LogMsg "$ErrMessage\n";
+  exit(1);
 }
 
-my $LastWebPatchId = 0;
-foreach my $Patch (@{CreatePatches()->GetItems()})
-{
-  my $WebPatchId = $Patch->WebPatchId;
-  if (defined $WebPatchId and $LastWebPatchId < $WebPatchId)
-  {
-    $LastWebPatchId = $WebPatchId;
-  }
-}
-
-while (1)
-{
-  $LastWebPatchId++;
-  my $NewPatch = "$DataDir/webpatches/$LastWebPatchId";
-  last if (!-f $NewPatch);
-
-  my $FileNameRandomPart = GenerateRandomString(32);
-  while (-e "$DataDir/staging/${FileNameRandomPart}_patch_$LastWebPatchId")
-  {
-    $FileNameRandomPart = GenerateRandomString(32);
-  }
-  my $StagingFileName = "$DataDir/staging/${FileNameRandomPart}_patch_$LastWebPatchId";
-
-  if (!copy($NewPatch, $StagingFileName))
-  {
-    LogMsg "Unable to copy '$NewPatch' to '$StagingFileName': $!\n";
-    exit 1;
-  }
-
-  WinePatchWebSubmission("${FileNameRandomPart}_patch_$LastWebPatchId", $LastWebPatchId);
-  LogMsg "Added wine-patches patch $LastWebPatchId\n";
-  if (defined $MaxCount)
-  {
-    $MaxCount--;
-    last if ($MaxCount == 0);
-  }
-}
-
-exit;
+exit(0);
diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm
index 09191ee..1f93db4 100644
--- a/testbot/lib/WineTestBot/Engine/Notify.pm
+++ b/testbot/lib/WineTestBot/Engine/Notify.pm
@@ -233,9 +233,7 @@ sub WinePatchMLSubmission
 
 sub WinePatchWebSubmission
 {
-  my ($FileName, $WebPatchId) = @_;
-
-  my $Reply = SendCmdReceiveReply("winepatchwebsubmission $FileName $WebPatchId\n");
+  my $Reply = SendCmdReceiveReply("winepatchwebsubmission\n");
   if (length($Reply) < 1)
   {
     return "Unrecognized reply received from engine";