[PATCH 1/4] testbot: Rename the patch analyzer to GetPatchImpacts().
The returned structure is called $Impacts everywhere so this makes the naming more consistent. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/bin/WineRunBuild.pl | 2 +- testbot/lib/Build/Utils.pm | 2 +- testbot/lib/WineTestBot/PatchUtils.pm | 6 +++--- testbot/lib/WineTestBot/Patches.pm | 4 ++-- testbot/web/Submit.pl | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/testbot/bin/WineRunBuild.pl b/testbot/bin/WineRunBuild.pl index 11b41c67a..d9bedb533 100755 --- a/testbot/bin/WineRunBuild.pl +++ b/testbot/bin/WineRunBuild.pl @@ -446,7 +446,7 @@ FatalTAError(undef, $TAError) if (defined $TAError); # Grab the executables for the next steps # -my $Impacts = GetPatchImpact($FileName, "nounit"); +my $Impacts = GetPatchImpacts($FileName, "nounit"); my $StepDir = $Step->CreateDir(); foreach my $TestInfo (values %{$Impacts->{Tests}}) { diff --git a/testbot/lib/Build/Utils.pm b/testbot/lib/Build/Utils.pm index fd1b8467c..38a801260 100644 --- a/testbot/lib/Build/Utils.pm +++ b/testbot/lib/Build/Utils.pm @@ -113,7 +113,7 @@ sub ApplyPatch($$) return undef; } - my $Impacts = GetPatchImpact($PatchFile, "nounits"); + my $Impacts = GetPatchImpacts($PatchFile, "nounits"); if ($Impacts->{MakeMakefiles}) { InfoMsg "\nRunning make_makefiles\n"; diff --git a/testbot/lib/WineTestBot/PatchUtils.pm b/testbot/lib/WineTestBot/PatchUtils.pm index 7049694aa..4a1761afb 100644 --- a/testbot/lib/WineTestBot/PatchUtils.pm +++ b/testbot/lib/WineTestBot/PatchUtils.pm @@ -31,7 +31,7 @@ the Wine builds. =cut use Exporter 'import'; -our @EXPORT = qw(GetPatchImpact UpdateWineData GetBuildTimeout); +our @EXPORT = qw(GetPatchImpacts UpdateWineData GetBuildTimeout); use List::Util qw(min max); @@ -249,7 +249,7 @@ sub _HandleFile($$$) =pod =over 12 -=item C<GetPatchImpact()> +=item C<GetPatchImpacts()> 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 @@ -258,7 +258,7 @@ configure, whether it impacts the tests, etc. =back =cut -sub GetPatchImpact($;$$) +sub GetPatchImpacts($;$$) { my ($PatchFileName, $NoUnits, $PastImpacts) = @_; diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm index 390b1611c..a052c5a8c 100644 --- a/testbot/lib/WineTestBot/Patches.pm +++ b/testbot/lib/WineTestBot/Patches.pm @@ -133,8 +133,8 @@ sub Submit($$$) my ($self, $PatchFileName, $IsSet) = @_; my $PastImpacts; - $PastImpacts = GetPatchImpact($PatchFileName) if ($IsSet); - my $Impacts = GetPatchImpact("$DataDir/patches/" . $self->Id, undef, $PastImpacts); + $PastImpacts = GetPatchImpacts($PatchFileName) if ($IsSet); + my $Impacts = GetPatchImpacts("$DataDir/patches/" . $self->Id, undef, $PastImpacts); if (!$Impacts->{WineBuild} and !$Impacts->{ModuleBuild} and !$Impacts->{TestBuild}) diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl index 621b01728..fe101042c 100644 --- a/testbot/web/Submit.pl +++ b/testbot/web/Submit.pl @@ -597,7 +597,7 @@ sub DetermineFileType($$) my ($ErrMessage, $ExeBase, $TestUnit); if ($FileType eq "unknown") { - my $Impacts = GetPatchImpact($FileName); + my $Impacts = GetPatchImpacts($FileName); if ($Impacts->{UnitCount} == 0) { $ErrMessage = "Patch doesn't affect tests"; @@ -781,7 +781,7 @@ sub OnSubmit($) if ($FileType eq "patch") { my $TmpStagingFullPath = $self->GetTmpStagingFullPath($BaseName); - $Impacts = GetPatchImpact($TmpStagingFullPath); + $Impacts = GetPatchImpacts($TmpStagingFullPath); } my $BuildStep; -- 2.18.0
The goal was to be able to use it on systems that don't have the winefiles.txt file. However nowadays all systems have access to it. Furthermore the amount of work saved is not really significant so there is no reason for the extra complexity. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/bin/WineRunBuild.pl | 2 +- testbot/lib/Build/Utils.pm | 2 +- testbot/lib/WineTestBot/PatchUtils.pm | 9 +++++---- testbot/lib/WineTestBot/Patches.pm | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/testbot/bin/WineRunBuild.pl b/testbot/bin/WineRunBuild.pl index d9bedb533..ec36b629a 100755 --- a/testbot/bin/WineRunBuild.pl +++ b/testbot/bin/WineRunBuild.pl @@ -446,7 +446,7 @@ FatalTAError(undef, $TAError) if (defined $TAError); # Grab the executables for the next steps # -my $Impacts = GetPatchImpacts($FileName, "nounit"); +my $Impacts = GetPatchImpacts($FileName); my $StepDir = $Step->CreateDir(); foreach my $TestInfo (values %{$Impacts->{Tests}}) { diff --git a/testbot/lib/Build/Utils.pm b/testbot/lib/Build/Utils.pm index 38a801260..2fe7a9778 100644 --- a/testbot/lib/Build/Utils.pm +++ b/testbot/lib/Build/Utils.pm @@ -113,7 +113,7 @@ sub ApplyPatch($$) return undef; } - my $Impacts = GetPatchImpacts($PatchFile, "nounits"); + my $Impacts = GetPatchImpacts($PatchFile); if ($Impacts->{MakeMakefiles}) { InfoMsg "\nRunning make_makefiles\n"; diff --git a/testbot/lib/WineTestBot/PatchUtils.pm b/testbot/lib/WineTestBot/PatchUtils.pm index 4a1761afb..4ce5bf40c 100644 --- a/testbot/lib/WineTestBot/PatchUtils.pm +++ b/testbot/lib/WineTestBot/PatchUtils.pm @@ -193,7 +193,6 @@ sub _HandleFile($$$) } return; } - return if ($Impacts->{NoUnits}); if (!$Tests->{$Module}->{Files}) { @@ -258,15 +257,17 @@ configure, whether it impacts the tests, etc. =back =cut -sub GetPatchImpacts($;$$) +sub GetPatchImpacts($;$) { - my ($PatchFileName, $NoUnits, $PastImpacts) = @_; + my ($PatchFileName, $PastImpacts) = @_; my $fh; return undef if (!open($fh, "<", $PatchFileName)); my $Impacts = { - NoUnits => $NoUnits, + # Number of test units impacted either directly, or indirectly by a module + # patch. + ModuleUnitCount => 0, # Number of patched test units. UnitCount => 0, # The modules that need a rebuild, even if only for the tests. diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm index a052c5a8c..5024ed489 100644 --- a/testbot/lib/WineTestBot/Patches.pm +++ b/testbot/lib/WineTestBot/Patches.pm @@ -134,7 +134,7 @@ sub Submit($$$) my $PastImpacts; $PastImpacts = GetPatchImpacts($PatchFileName) if ($IsSet); - my $Impacts = GetPatchImpacts("$DataDir/patches/" . $self->Id, undef, $PastImpacts); + my $Impacts = GetPatchImpacts("$DataDir/patches/" . $self->Id, $PastImpacts); if (!$Impacts->{WineBuild} and !$Impacts->{ModuleBuild} and !$Impacts->{TestBuild}) -- 2.18.0
Even if the patch provides an updated configure file the TestBot needs to be able to recreate it in case another patch does not provide it. Furthermore skipping the autoconf steps correctly is complex when a patchset changes configure.ac in part 1 and provides configure in part 2, or provides both in part 1 and only some other configure.ac change in part 2, etc. So it's better to just run autoconf whenever configure.ac is patched. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- We could also decide to rerun autoconf even if only configure is updated. This may help detect patches that should have modified configure.ac instead but only if there is code that really needs the new configure (rather than disable some functionality as is usual). It would also defeat testing of configure changes following an autoconf update. So I'm not sure that would be better. testbot/lib/Build/Utils.pm | 2 +- testbot/lib/WineTestBot/PatchUtils.pm | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/testbot/lib/Build/Utils.pm b/testbot/lib/Build/Utils.pm index 2fe7a9778..2e6cc853b 100644 --- a/testbot/lib/Build/Utils.pm +++ b/testbot/lib/Build/Utils.pm @@ -125,7 +125,7 @@ sub ApplyPatch($$) } } - if ($Impacts->{Autoconf} && !$Impacts->{HasConfigure}) + if ($Impacts->{Autoconf}) { InfoMsg "\nRunning autoconf\n"; system("cd '$DataDir/$Dir' && set -x && autoconf"); diff --git a/testbot/lib/WineTestBot/PatchUtils.pm b/testbot/lib/WineTestBot/PatchUtils.pm index 4ce5bf40c..476757f38 100644 --- a/testbot/lib/WineTestBot/PatchUtils.pm +++ b/testbot/lib/WineTestBot/PatchUtils.pm @@ -317,10 +317,6 @@ sub GetPatchImpacts($;$) { $Impacts->{WineBuild} = $Impacts->{Autoconf} = 1; } - elsif ($Line =~ m=^--- \w+/configure$=) - { - $Impacts->{WineBuild} = $Impacts->{HasConfigure} = 1; - } elsif ($Line =~ m=^--- \w+/tools/make_makefiles$=) { $Impacts->{WineBuild} = $Impacts->{MakeMakefiles} = 1; -- 2.18.0
This will let GetPatchImpacts() callers make optimisations like passing the module name to WineTest instead of the full test unit list. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/lib/WineTestBot/PatchUtils.pm | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/testbot/lib/WineTestBot/PatchUtils.pm b/testbot/lib/WineTestBot/PatchUtils.pm index 476757f38..9e0e68980 100644 --- a/testbot/lib/WineTestBot/PatchUtils.pm +++ b/testbot/lib/WineTestBot/PatchUtils.pm @@ -353,7 +353,6 @@ sub GetPatchImpacts($;$) foreach my $TestInfo (values %{$Impacts->{Tests}}) { # For each module, identify modifications to non-C files and helper dlls - my $AllUnits; foreach my $File (keys %{$TestInfo->{Files}}) { # Skip unmodified files @@ -363,7 +362,7 @@ sub GetPatchImpacts($;$) if ($Base !~ s/(?:\.c|\.spec)$//) { # Any change to a non-C non-Spec file can potentially impact all tests - $AllUnits = 1; + $TestInfo->{All} = 1; last; } if (exists $TestInfo->{Files}->{"$Base.spec"} and @@ -371,7 +370,7 @@ sub GetPatchImpacts($;$) $TestInfo->{Files}->{"$Base.spec"})) { # Any change to a helper dll can potentially impact all tests - $AllUnits = 1; + $TestInfo->{All} = 1; last; } } @@ -380,7 +379,7 @@ sub GetPatchImpacts($;$) foreach my $File (keys %{$TestInfo->{Files}}) { # Skip unmodified files - next if (!$AllUnits and !$TestInfo->{Files}->{$File}); + next if (!$TestInfo->{All} and !$TestInfo->{Files}->{$File}); my $Base = $File; # Non-C files are not test units @@ -388,7 +387,7 @@ sub GetPatchImpacts($;$) # Helper dlls are not test units next if (exists $TestInfo->{Files}->{"$Base.spec"}); - if (($AllUnits or $TestInfo->{Files}->{$File}) and + if (($TestInfo->{All} or $TestInfo->{Files}->{$File}) and $TestInfo->{Files}->{$File} ne "rm") { # Only new/modified test units are impacted -- 2.18.0
participants (1)
-
Francois Gouget