Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When a port on quarterly is deleted, we get a sanity test failure #528

Open
dlangille opened this issue Jan 5, 2024 · 11 comments
Open

When a port on quarterly is deleted, we get a sanity test failure #528

dlangille opened this issue Jan 5, 2024 · 11 comments
Assignees

Comments

@dlangille
Copy link
Contributor

dlangille commented Jan 5, 2024

re https://cgit.freebsd.org/ports/commit/?h=2024Q1&id=9480ee435d739b109e910142f74e1338ed7d8151

re: https://dev.freshports.org/sanity_test_failures.php?message_id=9480ee435d739b109e910142f74e1338ed7d8151

x11/hyprland-share-picker:

I did not find a Makefile for this port, and none was mentioned in the
commit.  If a repocopy has been done, please ignore this message. This
command (FreshPorts code 1):

/usr/local/bin/sudo /usr/sbin/jexec freshports /make-port.sh /usr/ports
x11/hyprland-share-picker
2>/tmp/FreshPorts.x11.hyprland-share-picker.make-error.2024.1.5.15.12.44.21
623

produced this error:

Error message is: cd: /usr/ports/x11/hyprland-share-picker: No such file or
directory
make: cannot open /usr/ports/x11/hyprland-share-picker/Makefile.
Make results are : 
make: stopped in /

The above message comes from port.pm::_ExtractValuesFromMakefile which is called by port.pm::RefreshFromFiles

verifyport.pm invokes port.pm::RefreshFromFiles from two locations:

  • RefreshAllPortsTouchedByCommit
  • RefreshAllSlavePortsOfPortsTouchedByCommit

xml_munge_git.pm invokes both of those functions:

                if ($FetchOK) {
                        if ($refresh_ports) {
                                #  parameters:                                                        $Repository           $CommitBranch        $CommitLogPortsRef $dbh
                                $ErrorFound = FreshPorts::VerifyPort::RefreshAllPortsTouchedByCommit($Updates{repository}, $Updates{branch_git}, \%CommitLogPorts, $self->{dbh});

                                if (!$ErrorFound) {
                                        $ErrorFound = FreshPorts::VerifyPort::RefreshAllSlavePortsOfPortsTouchedByCommit($Updates{repository}, $Updates{branch_git}, \%CommitLogPorts, $self->{dbh}, 'git');
                                }

                                if (!$ErrorFound) {
                                        $ErrorFound = FreshPorts::VerifyPort::MarkVulnerableCommits(\%CommitLogPorts, $self->{dbh});
                                }

                                $self->notify_observers($FreshPorts::Messages::PortsRefreshed, (message_id => $Updates{commit_hash}, CommitLogPorts => \%CommitLogPorts) );

                        }
                }

Based on the logs, I'm looking at RefreshAllPortsTouchedByCommit()

@dlangille dlangille self-assigned this Jan 5, 2024
@dlangille
Copy link
Contributor Author

dlangille commented Jan 5, 2024

I notice x11/hyprland-share-picker on branch 2024Q1 is still marked as active.

freshports.dev=# select *, element_pathname(id) from element where id = 1390030;
   id    |            name             | parent_id | directory_file_flag | status |                    element_pathname                    
---------+-----------------------------+-----------+---------------------+--------+--------------------------------------------------------
 1390030 | xdg-desktop-portal-hyprland |   1390029 | D                   | A      | /ports/branches/2024Q1/x11/xdg-desktop-portal-hyprland
(1 row)

This messes up verifyport.pm::_CompileListOfPorts()

Current theory: we're having trouble marking a port on a branch as deleted.

@dlangille
Copy link
Contributor Author

Found in logs:

# # # # Deleting deleted ports # # # #

deleting : port = x11/hyprland-share-picker, port_id = '115850', ' element_id = 1390035'
# # # # Finished deleting deleted ports # # # #

Which port is that?

freshports.dev=# select id, element_pathname(element_id) from ports where element_id = 1390035;
   id   |                 element_pathname                 
--------+--------------------------------------------------
 115850 | /ports/branches/2024Q1/x11/hyprland-share-picker
(1 row)

freshports.dev=# 

Yeah, that's the right one.

And it's marked as deleted:

freshports.dev=# select *, element_pathname(id) from element where id = 1390035;
   id    |         name          | parent_id | directory_file_flag | status |                 element_pathname                 
---------+-----------------------+-----------+---------------------+--------+--------------------------------------------------
 1390035 | hyprland-share-picker |   1390029 | D                   | D      | /ports/branches/2024Q1/x11/hyprland-share-picker
(1 row)

That one is properly deleted. So where is the problem?

@dlangille
Copy link
Contributor Author

dlangille commented Jan 7, 2024

We do set it deleted in verifyport.pm::_DeleteDeletedPorts()

NOTE: the element is being deleted, the $port local variable may not be.

sub _DeleteDeletedPorts($;$) {
        #
        # For each deleted port, delete the element which corresponds to that port
        #

        my $PortsRef = shift;
        my %Ports    = %{$PortsRef};
        my $dbh      = shift;

        my $element  = FreshPorts::Element->new($dbh);

        #
        # refresh each and every port we are told about
        #
        print "# # # # Deleting deleted ports # # # #\n\n";
        while (my ($portname, $port) = each %Ports) {
                if (defined($port->{deleted})) {
                        print "deleting : port = $portname, port_id = '$port->{id}', ' element_id = $port->{element_id}'\n";

                        $element->{id} = $port->{element_id};
                        if (defined($element->FetchByID())) {
                                $element->{status} = $FreshPorts::Element::Deleted;
                                $element->save();
                        }
                }
        }
        print "# # # # Finished deleting deleted ports # # # #\n\n";
}

@dlangille
Copy link
Contributor Author

This is an instance where you go round and round looking at stuff because you had it in your head two days ago, but not today.

@dlangille
Copy link
Contributor Author

From verifyport.pm::RefreshAllPortsTouchedByCommit().

Clearly, if the port is not active, it will not be refreshed.

        while (my ($portname, $commit_log_ports) = each %CommitLogPorts) {
                $port = $commit_log_ports->{port};
                print "port = $portname, port_id = '$port->{id}', category_id='$port->{category_id}', needs_refresh='$commit_log_ports->{needs_refresh}'\n";

                #
                # Sometimes a port can be deleted in one commit, and a later
                # commit will remove a missed file.  If this port is deleted, don't refresh it.
                # If we don't need to refresh it, we don't need to save it.
                #
                if ($port->IsActive()) {
                        $error = $port->RefreshFromFiles($Repository, $CommitBranch);
                } else {
                        print "This port is deleted: not refreshing.\n";
                        $error = 0;
                }

                if (!$error) {
                        if ($port->IsActive()) {
                                # after [perhaps] refreshing from the files, save the results
                                $port->save($CommitBranch);  # perhaps we need two types of saves.  One after refresh, one not after refresh.
                        } else {
                                print "This port is deleted: not saving.\n";
                        }

                        print "Updating commit_log_ports\n";

@dlangille
Copy link
Contributor Author

Found the main clue. When we find that a port has been delete from the repo, we set the deleted flag.

                                                if ($extra eq $FreshPorts::Constants::FILE_MAKEFILE && ($action eq $FreshPorts::Constants::REMOVE || $action eq $FreshPorts::Constants::DELETE)) {
                                                        #
                                                        # EDIT 2020-07-30 - for git processing, we want to delete the parent of $extra when we detect that the
                                                        # port Makefile is being deleted. see https://news.freshports.org/2020/07/29/git-changing-libraries-gave-us-new-xml-options/
                                                        # This should be straight forward.
                                                        #
                                                        # we are deleted (local value, never actually saved to db)
                                                        #
                                                        $port->{deleted} = 1;
                                                        print "THIS PORT HAS BEEN DELETED\n";
                                                }

@dlangille
Copy link
Contributor Author

dlangille commented Jan 7, 2024

However, two comments up, we check $port->IsActive(), clearly different.

Bonus: verifyport.pm is the only place that function is used:

[17:34 dev-ingress01 dan ~/modules] % grep -r  IsActive  * ~/scripts 
grep: config.pm: Permission denied
port.pm:sub IsActive {
port.pm.see.325:sub IsActive {
verifyport.pm:		if ($port->IsActive()) {
verifyport.pm:			if ($port->IsActive()) {
grep: /usr/home/dan/scripts/.#freebsd-cvs.sh: No such file or directory

Said function is in port.pm:

sub IsActive {
        my $this = shift;

        return ($this->{status} eq $FreshPorts::Element::Active);
}

status is set when the port is read from the database.

@dlangille
Copy link
Contributor Author

To test this commit again:

-- delete the commit
begin; delete from commit_log where message_id = '9480ee435d739b109e910142f74e1338ed7d8151';
-- update on the branch
update element set status = 'A' where id = 1390035;
-- update on head
update element set status = 'A' where id = 1328057;

@dlangille
Copy link
Contributor Author

I think this has fixed it. Let's run with it for a few days before saving it.

Index: port.pm
===================================================================
--- port.pm	(revision 5982)
+++ port.pm	(working copy)
@@ -572,7 +572,7 @@
 		# This preserves commit history when a port is being renamed, but it makes life difficultJailShowConfigScript
 		# for FreshPorts, which only tracks commits.
 		# We need the trailing space because there will be another message added right after this one.
-		FreshPorts::CommitterOptIn::RecordErrorDetails("$this->{category}/$this->{name}", "I did not find a Makefile for this port, and none was mentioned in the commit.  If a repocopy has been done, please ignore this message. ");
+		FreshPorts::CommitterOptIn::RecordErrorDetails("$this->{category}/$this->{name}", "I did not find a Makefile for this port. ");
 	}
 
 	my $TmpFile = FreshPorts::Utilities::TmpFileName("$this->{category}.$this->{name}.make-error");
@@ -1334,7 +1334,8 @@
 	#
 	# Let's just use this for now.  See how it goes.
 	#
-	if (!defined($this->{deleted})) {
+#	if (!defined($this->{deleted})) {
+	if (!$this->IsDeleted()) {
 		return 7;
 	} else {
 		return 0
@@ -1378,6 +1379,12 @@
   # convert from YYYY-MM-DD format into variables
   my ($year, $mon, $mday) = split /-/, $string;
 
+  # e.g. 20240101 (no dashes, for example)
+  #
+  if (!defined($year) || !defined($mon) || !defined($mday)) {
+    return undef;
+  }
+
   # create a string
   my $test = strftime("%Y-%m-%d", 0, 0, 0, $mday, $mon - 1, $year - 1900);
   
Index: verifyport.pm
===================================================================
--- verifyport.pm	(revision 6007)
+++ verifyport.pm	(working copy)
@@ -213,7 +213,9 @@
 							#
 							# we are deleted (local value, never actually saved to db)
 							#
-							$port->{deleted} = 1;
+#							$port->{deleted} = 1;
+							# instead of settting $port->{deleted} = 1;, try this: re https://github.com/FreshPorts/freshports/issues/528
+							$port->SetDeleted();
 							print "THIS PORT HAS BEEN DELETED\n";
 						}
 					}
@@ -798,7 +800,9 @@
 	#
 	print "# # # # Deleting deleted ports # # # #\n\n";
 	while (my ($portname, $port) = each %Ports) {
-		if (defined($port->{deleted})) {
+		# It may be that $port->{deleted} is distinct from $port->IsDeleted() - but we are about to find out.
+#		if ($port->{deleted})) {
+		if ($port->IsDeleted()) {
 			print "deleting : port = $portname, port_id = '$port->{id}', ' element_id = $port->{element_id}'\n";
 
 			$element->{id} = $port->{element_id};
[1:06 dev-ingress01 dan ~/modules] % 

@dlangille
Copy link
Contributor Author

now packaged.

Installed on test, dev, and stage.

@dlangille
Copy link
Contributor Author

With 8e7c184e8620e1568983be5b8beabd4047650f70 this problem has resurfaced. The main problem: Deleting deleted ports is not deleting any ports.

@dlangille dlangille reopened this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant