[5/9] pakfire: Optimize upgradecore function

Message ID 20220309225655.4472-6-robin.roevens@disroot.org
State Superseded
Headers
Series pakfire: remove dup. code + seperate ui/logic |

Commit Message

Robin Roevens March 9, 2022, 10:56 p.m. UTC
  upgradecore function should just upgrade the core:
Moved check if upgrade is necessary to pakfire upgrade code, removing
code from upgradecore function duplicating codedbinfo workings.
Also adding more vebosity to pakfire upgrade.

Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
---
 src/pakfire/lib/functions.pl | 47 +++++++++++++++---------------------
 src/pakfire/pakfire          | 16 +++++++++++-
 2 files changed, 35 insertions(+), 28 deletions(-)
  

Comments

Michael Tremer March 21, 2022, 4:24 p.m. UTC | #1
Hello,

> On 9 Mar 2022, at 22:56, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> upgradecore function should just upgrade the core:
> Moved check if upgrade is necessary to pakfire upgrade code, removing
> code from upgradecore function duplicating codedbinfo workings.
> Also adding more vebosity to pakfire upgrade.
> 
> Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> ---
> src/pakfire/lib/functions.pl | 47 +++++++++++++++---------------------
> src/pakfire/pakfire          | 16 +++++++++++-
> 2 files changed, 35 insertions(+), 28 deletions(-)
> 
> diff --git a/src/pakfire/lib/functions.pl b/src/pakfire/lib/functions.pl
> index 1e2729485..6287367f5 100644
> --- a/src/pakfire/lib/functions.pl
> +++ b/src/pakfire/lib/functions.pl
> @@ -735,35 +735,28 @@ sub setuppak {
> }
> 
> sub upgradecore {
> -	getcoredb("noforce");
> -	eval(`grep "core_" $Conf::dbdir/lists/core-list.db`);
> -	if ("$core_release" > "$Conf::core_mine") {
> -		# Safety check for lazy testers:
> -		# Before we upgrade to the latest release, we re-install the previous release
> -		# to make sure that the tester has always been on the latest version.
> -		my $tree = &get_tree();
> -		$Conf::core_mine-- if ($tree eq "testing" || $tree eq "unstable");
> -
> -		message("CORE UPGR: Upgrading from release $Conf::core_mine to $core_release");
> -
> -		my @seq = `seq $Conf::core_mine $core_release`;
> -		shift @seq;
> -		my $release;
> -		foreach $release (@seq) {
> -			chomp($release);
> -			getpak("core-upgrade-$release");
> -		}
> -
> -		foreach $release (@seq) {
> -			chomp($release);
> -			upgradepak("core-upgrade-$release");
> -		}
> -
> -		system("echo $core_release > $Conf::coredir/mine");
> +	# Safety check for lazy testers:
> +	# Before we upgrade to the latest release, we re-install the previous release
> +	# to make sure that the tester has always been on the latest version.
> +	my $tree = &get_tree();
> +	$Conf::core_mine-- if ($tree eq "testing" || $tree eq "unstable");
> 
> -	} else {
> -		message("CORE ERROR: No new upgrades available. You are on release $Conf::core_mine.");
> +	message("CORE UPGR: Upgrading from release $Conf::core_mine to $core_release");
> +	
> +	my @seq = `seq $Conf::core_mine $core_release`;

Can we remove this shell command call? This should be easily replaced with native Perl.

> +	shift @seq;
> +	my $release;
> +	foreach $release (@seq) {
> +		chomp($release);
> +		getpak("core-upgrade-$release");
> 	}
> +	
> +	foreach $release (@seq) {
> +		chomp($release);
> +		upgradepak("core-upgrade-$release");
> +	}
> +	
> +	system("echo $core_release > $Conf::coredir/mine");
> }
> 
> sub isinstalled {
> diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire
> index f23110cf5..2fb9adce7 100644
> --- a/src/pakfire/pakfire
> +++ b/src/pakfire/pakfire
> @@ -266,7 +266,21 @@
> 			$use_color = "$Pakfire::color{'lightpurple'}";
> 		}
> 
> -		&Pakfire::upgradecore();
> +		&Pakfire::message("CORE INFO: Checking for Core updates...");
> +
> +		### Make sure that the core db is not outdated. 
> +		&Pakfire::getcoredb("noforce");
> +		my %coredb = &Pakfire::coredbinfo();
> +
> +		if (defined $coredb{'AvailableRelease'}) {
> +			&Pakfire::upgradecore();
> +		} else {
> +			&Pakfire::message("CORE WARN: No new Core upgrades available. You are on release ".$coredb{'Release'});

Here, the Core Update is being called Core Upgrade. That might be coming from somewhere else. In the end we should probably reword lots of the error messages to make them consistent and easy to understand.

> +		}
> +
> +		&Pakfire::message("PAKFIRE INFO: Checking for package updates...");
> +		### Make sure that the package list is not outdated. 
> +		&Pakfire::dbgetlist("noforce");
> 		
> 		my @deps = ();
> 		if (my %upgradepaks = &Pakfire::dblist("upgrade")) {
> -- 
> 2.34.1
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
  
Robin Roevens March 22, 2022, 12:58 p.m. UTC | #2
Michael Tremer schreef op ma 21-03-2022 om 16:24 [+0000]:
> Hello,
> 
> > On 9 Mar 2022, at 22:56, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > 
> > upgradecore function should just upgrade the core:
> > Moved check if upgrade is necessary to pakfire upgrade code,
> > removing
> > code from upgradecore function duplicating codedbinfo workings.
> > Also adding more vebosity to pakfire upgrade.
> > 
> > Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> > ---
> > src/pakfire/lib/functions.pl | 47 +++++++++++++++------------------
> > ---
> > src/pakfire/pakfire          | 16 +++++++++++-
> > 2 files changed, 35 insertions(+), 28 deletions(-)
> > 
> > diff --git a/src/pakfire/lib/functions.pl
> > b/src/pakfire/lib/functions.pl
> > index 1e2729485..6287367f5 100644
> > --- a/src/pakfire/lib/functions.pl
> > +++ b/src/pakfire/lib/functions.pl
> > @@ -735,35 +735,28 @@ sub setuppak {
> > }
> > 
> > sub upgradecore {
> > -       getcoredb("noforce");
> > -       eval(`grep "core_" $Conf::dbdir/lists/core-list.db`);
> > -       if ("$core_release" > "$Conf::core_mine") {
> > -               # Safety check for lazy testers:
> > -               # Before we upgrade to the latest release, we re-
> > install the previous release
> > -               # to make sure that the tester has always been on
> > the latest version.
> > -               my $tree = &get_tree();
> > -               $Conf::core_mine-- if ($tree eq "testing" || $tree
> > eq "unstable");
> > -
> > -               message("CORE UPGR: Upgrading from release
> > $Conf::core_mine to $core_release");
> > -
> > -               my @seq = `seq $Conf::core_mine $core_release`;
> > -               shift @seq;
> > -               my $release;
> > -               foreach $release (@seq) {
> > -                       chomp($release);
> > -                       getpak("core-upgrade-$release");
> > -               }
> > -
> > -               foreach $release (@seq) {
> > -                       chomp($release);
> > -                       upgradepak("core-upgrade-$release");
> > -               }
> > -
> > -               system("echo $core_release > $Conf::coredir/mine");
> > +       # Safety check for lazy testers:
> > +       # Before we upgrade to the latest release, we re-install
> > the previous release
> > +       # to make sure that the tester has always been on the
> > latest version.
> > +       my $tree = &get_tree();
> > +       $Conf::core_mine-- if ($tree eq "testing" || $tree eq
> > "unstable");
> > 
> > -       } else {
> > -               message("CORE ERROR: No new upgrades available. You
> > are on release $Conf::core_mine.");
> > +       message("CORE UPGR: Upgrading from release $Conf::core_mine
> > to $core_release");
> > +       
> > +       my @seq = `seq $Conf::core_mine $core_release`;
> 
> Can we remove this shell command call? This should be easily replaced
> with native Perl.
Strange.. I can remember I changed that to
($Conf::core_mine..$core_release) .. must have gone lost somewhere in
my drafts.
I will change it.

> 
> > +       shift @seq;
> > +       my $release;
> > +       foreach $release (@seq) {
> > +               chomp($release);
> > +               getpak("core-upgrade-$release");
> >         }
> > +       
> > +       foreach $release (@seq) {
> > +               chomp($release);
> > +               upgradepak("core-upgrade-$release");
> > +       }
> > +       
> > +       system("echo $core_release > $Conf::coredir/mine");
> > }
> > 
> > sub isinstalled {
> > diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire
> > index f23110cf5..2fb9adce7 100644
> > --- a/src/pakfire/pakfire
> > +++ b/src/pakfire/pakfire
> > @@ -266,7 +266,21 @@
> >                         $use_color =
> > "$Pakfire::color{'lightpurple'}";
> >                 }
> > 
> > -               &Pakfire::upgradecore();
> > +               &Pakfire::message("CORE INFO: Checking for Core
> > updates...");
> > +
> > +               ### Make sure that the core db is not outdated. 
> > +               &Pakfire::getcoredb("noforce");
> > +               my %coredb = &Pakfire::coredbinfo();
> > +
> > +               if (defined $coredb{'AvailableRelease'}) {
> > +                       &Pakfire::upgradecore();
> > +               } else {
> > +                       &Pakfire::message("CORE WARN: No new Core
> > upgrades available. You are on release ".$coredb{'Release'});
> 
> Here, the Core Update is being called Core Upgrade. That might be
> coming from somewhere else. In the end we should probably reword lots
> of the error messages to make them consistent and easy to understand.
Yes, also "pakfire update" vs "pakfire upgrade", possibly the reason
why it is called "core upgrade" here.
Should I change everything to 'update' ?
But then I think it would also be more clear if 'pakfire update' would
update the system instead of only the pakfire db?
So maybe we can 'rename' 'pakfire update' to 'pakfire refresh' and then
rename 'pakfire upgrade' to 'pakfire update'... (possibly keeping
'pakfire upgrade' as an alias for 'pakfire update' for some backward
compatibility)
I'm a bit reticent about this as it changes the existing meaning of
'pakfire update' but I think that it would be more consistent in the
end ?

> 
> > +               }
> > +
> > +               &Pakfire::message("PAKFIRE INFO: Checking for
> > package updates...");
> > +               ### Make sure that the package list is not
> > outdated. 
> > +               &Pakfire::dbgetlist("noforce");
> >                 
> >                 my @deps = ();
> >                 if (my %upgradepaks = &Pakfire::dblist("upgrade"))
> > {
> > -- 
> > 2.34.1
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
>
  
Michael Tremer March 22, 2022, 3:16 p.m. UTC | #3
Hey,

> On 22 Mar 2022, at 12:58, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> 
> Michael Tremer schreef op ma 21-03-2022 om 16:24 [+0000]:
>> Hello,
>> 
>>> On 9 Mar 2022, at 22:56, Robin Roevens <robin.roevens@disroot.org>
>>> wrote:
>>> 
>>> upgradecore function should just upgrade the core:
>>> Moved check if upgrade is necessary to pakfire upgrade code,
>>> removing
>>> code from upgradecore function duplicating codedbinfo workings.
>>> Also adding more vebosity to pakfire upgrade.
>>> 
>>> Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
>>> ---
>>> src/pakfire/lib/functions.pl | 47 +++++++++++++++------------------
>>> ---
>>> src/pakfire/pakfire          | 16 +++++++++++-
>>> 2 files changed, 35 insertions(+), 28 deletions(-)
>>> 
>>> diff --git a/src/pakfire/lib/functions.pl
>>> b/src/pakfire/lib/functions.pl
>>> index 1e2729485..6287367f5 100644
>>> --- a/src/pakfire/lib/functions.pl
>>> +++ b/src/pakfire/lib/functions.pl
>>> @@ -735,35 +735,28 @@ sub setuppak {
>>> }
>>> 
>>> sub upgradecore {
>>> -       getcoredb("noforce");
>>> -       eval(`grep "core_" $Conf::dbdir/lists/core-list.db`);
>>> -       if ("$core_release" > "$Conf::core_mine") {
>>> -               # Safety check for lazy testers:
>>> -               # Before we upgrade to the latest release, we re-
>>> install the previous release
>>> -               # to make sure that the tester has always been on
>>> the latest version.
>>> -               my $tree = &get_tree();
>>> -               $Conf::core_mine-- if ($tree eq "testing" || $tree
>>> eq "unstable");
>>> -
>>> -               message("CORE UPGR: Upgrading from release
>>> $Conf::core_mine to $core_release");
>>> -
>>> -               my @seq = `seq $Conf::core_mine $core_release`;
>>> -               shift @seq;
>>> -               my $release;
>>> -               foreach $release (@seq) {
>>> -                       chomp($release);
>>> -                       getpak("core-upgrade-$release");
>>> -               }
>>> -
>>> -               foreach $release (@seq) {
>>> -                       chomp($release);
>>> -                       upgradepak("core-upgrade-$release");
>>> -               }
>>> -
>>> -               system("echo $core_release > $Conf::coredir/mine");
>>> +       # Safety check for lazy testers:
>>> +       # Before we upgrade to the latest release, we re-install
>>> the previous release
>>> +       # to make sure that the tester has always been on the
>>> latest version.
>>> +       my $tree = &get_tree();
>>> +       $Conf::core_mine-- if ($tree eq "testing" || $tree eq
>>> "unstable");
>>> 
>>> -       } else {
>>> -               message("CORE ERROR: No new upgrades available. You
>>> are on release $Conf::core_mine.");
>>> +       message("CORE UPGR: Upgrading from release $Conf::core_mine
>>> to $core_release");
>>> +       
>>> +       my @seq = `seq $Conf::core_mine $core_release`;
>> 
>> Can we remove this shell command call? This should be easily replaced
>> with native Perl.
> Strange.. I can remember I changed that to
> ($Conf::core_mine..$core_release) .. must have gone lost somewhere in
> my drafts.
> I will change it.

Okay. Thank you.

> 
>> 
>>> +       shift @seq;
>>> +       my $release;
>>> +       foreach $release (@seq) {
>>> +               chomp($release);
>>> +               getpak("core-upgrade-$release");
>>>         }
>>> +       
>>> +       foreach $release (@seq) {
>>> +               chomp($release);
>>> +               upgradepak("core-upgrade-$release");
>>> +       }
>>> +       
>>> +       system("echo $core_release > $Conf::coredir/mine");
>>> }
>>> 
>>> sub isinstalled {
>>> diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire
>>> index f23110cf5..2fb9adce7 100644
>>> --- a/src/pakfire/pakfire
>>> +++ b/src/pakfire/pakfire
>>> @@ -266,7 +266,21 @@
>>>                         $use_color =
>>> "$Pakfire::color{'lightpurple'}";
>>>                 }
>>> 
>>> -               &Pakfire::upgradecore();
>>> +               &Pakfire::message("CORE INFO: Checking for Core
>>> updates...");
>>> +
>>> +               ### Make sure that the core db is not outdated. 
>>> +               &Pakfire::getcoredb("noforce");
>>> +               my %coredb = &Pakfire::coredbinfo();
>>> +
>>> +               if (defined $coredb{'AvailableRelease'}) {
>>> +                       &Pakfire::upgradecore();
>>> +               } else {
>>> +                       &Pakfire::message("CORE WARN: No new Core
>>> upgrades available. You are on release ".$coredb{'Release'});
>> 
>> Here, the Core Update is being called Core Upgrade. That might be
>> coming from somewhere else. In the end we should probably reword lots
>> of the error messages to make them consistent and easy to understand.
> Yes, also "pakfire update" vs "pakfire upgrade", possibly the reason
> why it is called "core upgrade" here.
> Should I change everything to 'update' ?

No, this is a little bit similar to apt-get which has an update and an upgrade command. That was the intention at least.

I would just change the name “Core Upgrade” to “Core Update” because that is what we call it. It is only an aesthetic change.

> But then I think it would also be more clear if 'pakfire update' would
> update the system instead of only the pakfire db?
> So maybe we can 'rename' 'pakfire update' to 'pakfire refresh' and then
> rename 'pakfire upgrade' to 'pakfire update'... (possibly keeping
> 'pakfire upgrade' as an alias for 'pakfire update' for some backward
> compatibility)

This would break people’s scripts, so I wouldn’t say this is worth it.

> I'm a bit reticent about this as it changes the existing meaning of
> 'pakfire update' but I think that it would be more consistent in the
> end ?
> 
>> 
>>> +               }
>>> +
>>> +               &Pakfire::message("PAKFIRE INFO: Checking for
>>> package updates...");
>>> +               ### Make sure that the package list is not
>>> outdated. 
>>> +               &Pakfire::dbgetlist("noforce");
>>>                 
>>>                 my @deps = ();
>>>                 if (my %upgradepaks = &Pakfire::dblist("upgrade"))
>>> {
>>> -- 
>>> 2.34.1
>>> 
>>> 
>>> -- 
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>> 
>> 
>> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
  

Patch

diff --git a/src/pakfire/lib/functions.pl b/src/pakfire/lib/functions.pl
index 1e2729485..6287367f5 100644
--- a/src/pakfire/lib/functions.pl
+++ b/src/pakfire/lib/functions.pl
@@ -735,35 +735,28 @@  sub setuppak {
 }
 
 sub upgradecore {
-	getcoredb("noforce");
-	eval(`grep "core_" $Conf::dbdir/lists/core-list.db`);
-	if ("$core_release" > "$Conf::core_mine") {
-		# Safety check for lazy testers:
-		# Before we upgrade to the latest release, we re-install the previous release
-		# to make sure that the tester has always been on the latest version.
-		my $tree = &get_tree();
-		$Conf::core_mine-- if ($tree eq "testing" || $tree eq "unstable");
-
-		message("CORE UPGR: Upgrading from release $Conf::core_mine to $core_release");
-
-		my @seq = `seq $Conf::core_mine $core_release`;
-		shift @seq;
-		my $release;
-		foreach $release (@seq) {
-			chomp($release);
-			getpak("core-upgrade-$release");
-		}
-
-		foreach $release (@seq) {
-			chomp($release);
-			upgradepak("core-upgrade-$release");
-		}
-
-		system("echo $core_release > $Conf::coredir/mine");
+	# Safety check for lazy testers:
+	# Before we upgrade to the latest release, we re-install the previous release
+	# to make sure that the tester has always been on the latest version.
+	my $tree = &get_tree();
+	$Conf::core_mine-- if ($tree eq "testing" || $tree eq "unstable");
 
-	} else {
-		message("CORE ERROR: No new upgrades available. You are on release $Conf::core_mine.");
+	message("CORE UPGR: Upgrading from release $Conf::core_mine to $core_release");
+	
+	my @seq = `seq $Conf::core_mine $core_release`;
+	shift @seq;
+	my $release;
+	foreach $release (@seq) {
+		chomp($release);
+		getpak("core-upgrade-$release");
 	}
+	
+	foreach $release (@seq) {
+		chomp($release);
+		upgradepak("core-upgrade-$release");
+	}
+	
+	system("echo $core_release > $Conf::coredir/mine");
 }
 
 sub isinstalled {
diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire
index f23110cf5..2fb9adce7 100644
--- a/src/pakfire/pakfire
+++ b/src/pakfire/pakfire
@@ -266,7 +266,21 @@ 
 			$use_color = "$Pakfire::color{'lightpurple'}";
 		}
 
-		&Pakfire::upgradecore();
+		&Pakfire::message("CORE INFO: Checking for Core updates...");
+
+		### Make sure that the core db is not outdated. 
+		&Pakfire::getcoredb("noforce");
+		my %coredb = &Pakfire::coredbinfo();
+
+		if (defined $coredb{'AvailableRelease'}) {
+			&Pakfire::upgradecore();
+		} else {
+			&Pakfire::message("CORE WARN: No new Core upgrades available. You are on release ".$coredb{'Release'});
+		}
+
+		&Pakfire::message("PAKFIRE INFO: Checking for package updates...");
+		### Make sure that the package list is not outdated. 
+		&Pakfire::dbgetlist("noforce");
 		
 		my @deps = ();
 		if (my %upgradepaks = &Pakfire::dblist("upgrade")) {