[PATCH 1/2] testbot/build: Add support for test helper dlls and make the patch parser reusable.
Don't mistake the helper dlls C source files for test units. Also correctly handle patches that add or remove (i.e. rename) a resource file or helper dll. And move the patch parser to its own module so it can be reused, including by processes that don't use / have access to the database. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- Note that helper dlls will only be correctly handled once both Reconfig.pl, for testlist.c on the build VM, and Patches, on the TestBot Engine, have been updated. However mixing the old and new versions of either should not be worse than the current situation and only impact patches to helper dlls. testbot/bin/build/Reconfig.pl | 19 ++- testbot/lib/WineTestBot/PatchUtils.pm | 224 ++++++++++++++++++++++++++ testbot/lib/WineTestBot/Patches.pm | 102 +----------- 3 files changed, 247 insertions(+), 98 deletions(-) create mode 100644 testbot/lib/WineTestBot/PatchUtils.pm diff --git a/testbot/bin/build/Reconfig.pl b/testbot/bin/build/Reconfig.pl index 7e267091b..7b0755a04 100755 --- a/testbot/bin/build/Reconfig.pl +++ b/testbot/bin/build/Reconfig.pl @@ -68,10 +68,21 @@ sub GitPull() return !1; } - system("( cd $DataDir/wine && ". - " ls dlls/*/tests/*.c programs/*/tests/*.c | ". - " egrep -v '/testlist.c\$' >../testlist.txt ". - ") >>$LogDir/Reconfig.log 2>&1"); + if (open(my $fh, ">", "$DataDir/testlist.txt")) + { + foreach my $TestFile (glob("$DataDir/wine/*/*/tests/*.c"), + glob("$DataDir/wine/*/*/tests/*.spec")) + { + next if ($TestFile =~ m=/testlist\.c$=); + $TestFile =~ s=^$DataDir/wine/==; + print $fh "$TestFile\n"; + } + close($fh); + } + else + { + LogMsg "Could not open 'testlist.txt' for writing: $!\n"; + } return 1; } diff --git a/testbot/lib/WineTestBot/PatchUtils.pm b/testbot/lib/WineTestBot/PatchUtils.pm new file mode 100644 index 000000000..74599ba30 --- /dev/null +++ b/testbot/lib/WineTestBot/PatchUtils.pm @@ -0,0 +1,224 @@ +# -*- Mode: Perl; perl-indent-level: 2; indent-tabs-mode: nil -*- +# Copyright 2018 Francois Gouget +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + +use strict; + +package WineTestBot::PatchUtils; + +=head1 NAME + +WineTestBot::PatchUtils - Parse and analyze patches. + +=head1 DESCRIPTION + +Provides functions to parse patches and figure out which impact they have on +the Wine builds. + +=cut + +use Exporter 'import'; +our @EXPORT = qw(GetPatchImpact); + +use WineTestBot::Config; + + +=pod +=over 12 + +=item C<GetTestList()> + +Returns a hashtable containing the list of the source files for a given module. +This structure is built from the latest/testlist.txt file. + +=back +=cut + +sub GetTestList() +{ + my $TestList = {}; + if (open(my $File, "<", "$DataDir/latest/testlist.txt")) + { + while (my $TestFileName = <$File>) + { + chomp $TestFileName; + if ($TestFileName =~ m~^\w+/([^/]+)/tests/([^/]+)$~) + { + my ($Module, $File) = ($1, $2); + $TestList->{$Module}->{$File} = 1; + } + } + close($File); + } + return $TestList; +} + +sub _AddTest($$$) +{ + my ($Impacts, $Path, $Change) = @_; + + return if ($Path !~ m~^(dlls|programs)/([^/]+)/tests/([^/\s]+)$~); + my ($Root, $Module, $File) = ($1, $2, $3); + + my $Tests = $Impacts->{Tests}; + if (!$Tests->{$Module}) + { + my $ExeBase = ($Root eq "programs") ? "${Module}.exe_test" : + "${Module}_test"; + $Tests->{$Module} = { + "Type" => "patch$Root", + "Module" => $Module, + "Path" => "$Root/$Module/tests", + "ExeBase" => $ExeBase, + }; + } + + # Assume makefile modifications may break the build but not the tests. + return if ($File eq "Makefile.in"); + + if (!$Tests->{$Module}->{Files}) + { + my $TestList = ( $Impacts->{TestList} ||= GetTestList() ); + foreach my $File (keys %{$TestList->{$Module}}) + { + $Tests->{$Module}->{Files}->{$File} = 0; # not modified + } + } + $Tests->{$Module}->{Files}->{$File} = $Change; +} + +sub dumpstr($) +{ + my ($Str) = @_; + return defined $Str ? "[$Str]" : "<undef>"; +} + +=pod +=over 12 + +=item C<GetPatchImpact()> + +Analyzes a patch and returns a hashtable describing the impact it has on the +Wine build: whether it requires updating the makefiles, re-running autoconf or +configure, whether it impacts the tests, etc. + +=back +=cut + +sub GetPatchImpact($) +{ + my ($PatchFileName) = @_; + + my $fh; + return undef if (!open($fh, "<", $PatchFileName)); + + my $Impacts = { Tests => {} }; + my ($Path, $Change); + while (my $Line = <$fh>) + { + if ($Line =~ m=^--- /dev/null$=) + { + $Change = "new"; + } + elsif ($Line =~ m~^--- \w+/([^/]+/[^/]+/tests/[^/\s]+)$~) + { + $Path = $1; + } + elsif ($Line =~ m~^\+\+\+ /dev/null$~) + { + _AddTest($Impacts, $Path, "rm") if (defined $Path); + $Path = undef; + $Change = ""; + } + elsif ($Line =~ m~^\+\+\+ \w+/(\w+/[^/]+/tests/[^/\s]+)$~) + { + _AddTest($Impacts, $1, $Change || "modify"); + $Path = undef; + $Change = ""; + } + else + { + $Path = undef; + $Change = ""; + } + } + close($fh); + + $Impacts->{ModuleCount} = 0; + $Impacts->{UnitCount} = 0; + foreach my $Module (keys %{$Impacts->{Tests}}) + { + my $TestInfo = $Impacts->{Tests}->{$Module}; + + # For each module, identify modifications to non-C files and helper dlls + my $AllUnits; + foreach my $File (keys %{$TestInfo->{Files}}) + { + # Skip unmodified files + next if (!$TestInfo->{Files}->{$File}); + + my $Base = $File; + if ($Base !~ s/(?:\.c|\.spec)$//) + { + # Any change to a non-C non-Spec file can potentially impact all tests + $AllUnits = 1; + last; + } + if (exists $TestInfo->{Files}->{"$Base.spec"} and + ($TestInfo->{Files}->{"$Base.c"} or + $TestInfo->{Files}->{"$Base.spec"})) + { + # Any change to a helper dll can potentially impact all tests + $AllUnits = 1; + last; + } + } + + $TestInfo->{Units} = {}; + foreach my $File (keys %{$TestInfo->{Files}}) + { + # Skip unmodified files + next if (!$AllUnits and !$TestInfo->{Files}->{$File}); + + my $Base = $File; + # Non-C files are not test units + next if ($Base !~ s/(?:\.c|\.spec)$//); + # Helper dlls are not test units + next if (exists $TestInfo->{Files}->{"$Base.spec"}); + + if ($AllUnits) + { + # All test units are impacted indirectly and must be rerun + $TestInfo->{Units}->{$Base} = 1; + } + elsif ($TestInfo->{Files}->{$File} and + $TestInfo->{Files}->{$File} ne "rm") + { + # Only new/modified test units are impacted + $TestInfo->{Units}->{$Base} = 1; + } + } + + $TestInfo->{UnitCount} = scalar(%{$TestInfo->{Units}}); + if ($TestInfo->{UnitCount}) + { + $Impacts->{ModuleCount}++; + $Impacts->{UnitCount} += $TestInfo->{UnitCount}; + } + } + + return $Impacts; +} diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm index b15668732..d4a5e0b17 100644 --- a/testbot/lib/WineTestBot/Patches.pm +++ b/testbot/lib/WineTestBot/Patches.pm @@ -41,6 +41,7 @@ use File::Basename; use WineTestBot::Config; use WineTestBot::Jobs; +use WineTestBot::PatchUtils; use WineTestBot::Users; use WineTestBot::Utils; use WineTestBot::VMs; @@ -109,36 +110,6 @@ sub FromSubmission($$) } -=pod -=over 12 - -=item C<GetTestList()> - -Returns a hashtable containing the list of the source files for a given module. -This structure is built from the latest/testlist.txt file. - -=back -=cut - -sub GetTestList() -{ - my $TestList = {}; - if (open(my $File, "<", "$DataDir/latest/testlist.txt")) - { - while (my $TestFileName = <$File>) - { - chomp $TestFileName; - if ($TestFileName =~ m~^(?:dlls|programs)/([^/]+)/tests/[^/]+\.c$~) - { - my $Module = $1; - push @{$TestList->{$Module}}, $TestFileName; - } - } - close($File); - } - return $TestList; -} - =pod =over 12 @@ -161,38 +132,8 @@ sub Submit($$$) { my ($self, $PatchFileName, $IsSet) = @_; - # See also OnSubmit() in web/Submit.pl - my (%Modules, %Deleted); - if (open(BODY, "<$DataDir/patches/" . $self->Id)) - { - my ($Line, $Modified); - while (defined($Line = <BODY>)) - { - if ($Line =~ m~^\-\-\- .*/((?:dlls|programs)/[^/]+/tests/[^/\s]+)~) - { - $Modified = $1; - } - elsif ($Line =~ m~^\+\+\+ .*/(dlls|programs)/([^/]+)/tests/([^/\s]+)~) - { - my ($FileType, $Module, $Unit) = ("patch$1", $2, $3); - # Assume makefile modifications may break the build but not the tests - next if ($Unit eq "Makefile.in"); - $Unit = "" if ($Unit !~ s/\.c$//); - $Modules{$Module}{$Unit} = $FileType; - } - elsif ($Line =~ m~^\+\+\+ /dev/null~ and defined $Modified) - { - $Deleted{$Modified} = 1; - } - else - { - $Modified = undef; - } - } - close BODY; - } - - if (! scalar(%Modules)) + my $Impacts = GetPatchImpact("$DataDir/patches/" . $self->Id); + if (!$Impacts->{UnitCount}) { $self->Disposition(($IsSet ? "Set" : "Patch") . " doesn't affect tests"); @@ -214,35 +155,11 @@ sub Submit($$$) $User = GetBatchUser(); } - my $TestList; - foreach my $Module (keys %Modules) - { - next if (!defined $Modules{$Module}{""}); - - # The patch modifies non-C files so rerun all that module's test units - $TestList = GetTestList() if (!$TestList); - next if (!defined $TestList->{$Module}); - - # If we don't find which tests to rerun then run the module test - # executable without argument. It probably won't work but will make the - # issue clearer to the developer. - my $FileType = $Modules{$Module}{""}; - foreach my $TestFileName (@{$TestList->{$Module}}) - { - if (!$Deleted{$TestFileName} and - $TestFileName =~ m~^(?:dlls|programs)/\Q$Module\E/tests/([^/]+)\.c$~) - { - my $Unit = $1; - $Modules{$Module}{$Unit} = $FileType; - delete $Modules{$Module}{""}; - } - } - } - my $Disposition = "Submitted job "; my $First = 1; - foreach my $Module (sort keys %Modules) + foreach my $Module (sort keys %{$Impacts->{Tests}}) { + my $TestInfo = $Impacts->{Tests}->{$Module}; my $Jobs = CreateJobs(); # Create a new job for this patch @@ -264,8 +181,7 @@ sub Submit($$$) # Create a link to the patch file in the staging dir my $StagingFileName = CreateNewLink($PatchFileName, "$DataDir/staging", "_patch.diff"); $NewStep->FileName(basename($StagingFileName)); - my @Keys = keys %{$Modules{$Module}}; - $NewStep->FileType($Modules{$Module}{$Keys[0]}); + $NewStep->FileType($TestInfo->{Type}); $NewStep->InStaging(1); $NewStep->Type("build"); $NewStep->DebugLevel(0); @@ -287,7 +203,7 @@ sub Submit($$$) return $ErrMessage; } - foreach my $Unit (sort keys %{$Modules{$Module}}) + foreach my $Unit (sort keys %{$TestInfo->{Units}}) { # Add 32 and 64-bit tasks foreach my $Bits ("32", "64") @@ -300,9 +216,7 @@ sub Submit($$$) # Create the corresponding Step $NewStep = $Steps->Add(); $NewStep->PreviousNo(1); - my $FileName = $Module; - $FileName .= ".exe" if ($Modules{$Module}{$Unit} eq "patchprograms"); - $FileName .= "_test"; + my $FileName = $TestInfo->{ExeBase}; $FileName .= "64" if ($Bits eq "64"); $NewStep->FileName("$FileName.exe"); $NewStep->FileType("exe$Bits"); -- 2.17.0
Also standardize on the TestUnit terminology. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/web/Submit.pl | 94 +++++++++---------------------------------- 1 file changed, 20 insertions(+), 74 deletions(-) diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl index 3b80841cc..cf850788d 100644 --- a/testbot/web/Submit.pl +++ b/testbot/web/Submit.pl @@ -35,6 +35,7 @@ use WineTestBot::Branches; use WineTestBot::Config; use WineTestBot::Jobs; use WineTestBot::Engine::Notify; +use WineTestBot::PatchUtils; use WineTestBot::Utils; use WineTestBot::VMs; @@ -510,7 +511,7 @@ sub DetermineFileType($$) my $ErrMessage = undef; my $FileType = "unknown"; my $DllBaseName = undef; - my $TestSet = undef; + my $TestUnit = undef; if (! sysopen(FH, $FileName, O_RDONLY)) { return ("Unable to open $FileName", "unknown", undef, undef); @@ -563,76 +564,21 @@ sub DetermineFileType($$) if ($FileType eq "unknown") { - if (open (FH, "<$FileName")) + my $Impacts = GetPatchImpact($FileName); + if ($Impacts->{UnitCount} == 0) { - my $Line; - my $PatchFound = !1; - my $PrevPlus = !1; - my $PrevMinus = !1; - while (defined($Line = <FH>)) - { - if ($Line =~ m~^\+\+\+ .*/(dlls|programs)/([^/]+)/tests/([^/\s]+)~) - { - $FileType = "patch$1"; - my $ThisDllBaseName = $2; - my $ThisTestSet = $3; - if ($ThisTestSet =~ m/^(.*)\.c$/) - { - $ThisTestSet = $1; - } - else - { - $ThisTestSet = undef; - } - - if ((defined($DllBaseName) && $DllBaseName ne $ThisDllBaseName) || - (defined($TestSet) && defined($ThisTestSet) && - $TestSet ne $ThisTestSet)) - { - $ErrMessage = "Patch contains changes to multiple tests"; - } - else - { - if (defined($ThisDllBaseName)) - { - $DllBaseName = $ThisDllBaseName; - } - if (defined($ThisTestSet)) - { - $TestSet = $ThisTestSet; - } - } - } - elsif ($Line =~ m/^\+\+\+ /) - { - if ($PrevMinus) - { - $PatchFound = 1; - $PrevMinus = !1; - } - $PrevPlus = 1; - } - elsif ($Line =~ m/^--- /) - { - if ($PrevPlus) - { - $PatchFound = 1; - $PrevPlus = !1; - } - $PrevMinus = 1; - } - else - { - $PrevPlus = !1; - $PrevMinus = !1; - } - } - close FH; - - if ($FileType eq "unknown" && $PatchFound) - { - $ErrMessage = "Patch doesn't affect tests"; - } + $ErrMessage = "Patch doesn't affect tests"; + } + elsif ($Impacts->{UnitCount} > 1) + { + $ErrMessage = "Patch contains changes to multiple tests"; + } + else + { + my $TestInfo = (values %{$Impacts->{Tests}})[0]; + $FileType = $TestInfo->{Type}; + $DllBaseName = $TestInfo->{Module}; + $TestUnit = (keys %{$TestInfo->{Units}})[0]; } } elsif ($FileType eq "dll32" || $FileType eq "dll64" || $FileType eq "zip") @@ -641,7 +587,7 @@ sub DetermineFileType($$) $FileType = "unknown"; } - return ($ErrMessage, $FileType, $DllBaseName, $TestSet); + return ($ErrMessage, $FileType, $DllBaseName, $TestUnit); } sub OnPage1Next($) @@ -686,7 +632,7 @@ sub OnPage1Next($) } close OUTFILE; - my ($ErrMessage, $FileType, $DllBaseName, $TestSet) = $self->DetermineFileType($StagingFile); + my ($ErrMessage, $FileType, $DllBaseName, $TestUnit) = $self->DetermineFileType($StagingFile); if (defined($ErrMessage)) { $self->{ErrField} = "File"; @@ -712,9 +658,9 @@ sub OnPage1Next($) } $self->{TestExecutable} .= "_test.exe"; } - if (defined($TestSet)) + if (defined($TestUnit)) { - $self->{CmdLineArg} = $TestSet; + $self->{CmdLineArg} = $TestUnit; } } else -- 2.17.0
participants (1)
-
Francois Gouget