indexing context: make it easier to log and report on indexing status #405

rjbs · 2023-04-29T09:28:42Z

This branch provides a common interface (the context object) for collecting and reporting on errors or warnings found during indexing. It also replaces the "set skip or return early" logic with an exception system, to allow deep-down checks to declare that a bit of indexing can't succeed.

I have compared the messages generated by test runs, and believe that while there are some changes, they are either neutral or improvements.

Here's more detail:

PAUSE::Indexer::Abort::Dist and PAUSE::Indexer::Abort::Package are introduced as exception types
PAUSE::Indexer::Context is added. We create one of these objects for each dist we index. This object tracks errors and warnings during indexing. It tracks whether packages have been indexed or not. It also can generate the exceptions above to abort indexing in progress.
PAUSE::Indexer::Errors is a registry of known errors that can happen. If we want to have a new kind of error, we add it here. We use that registry to provide the short and long form. This means that every error is now reported with both forms, whereas before some only had one.
Almost everything else is built on using the changes above. Previously, flags were set on pmfile or dists indicating problems. Later, something would check those flags. All the code that worked this way has been updated to signal an abort. The index_packages and maybe_index_dist methods have been updated to catch abort exceptions and handle them.

wolfsage

Wild!

I think we should probably test this on the same version of Perl pause is running on to be sure we haven't introduced more incompatibilities (like indented heredocs).

I mostly understand it but it's a lot to reason about.

I think this commit message needs rewording:

Code that wants to abort can $ctx->abort_indexing, which adds a
dist_error uses an exception to stop processing the dist further.

"which adds a dist_error uses an exception" I can't seem to parse

And the final commit regarding attempting to avoid partial indexing doesn't quite make sense. I'm reasonably sure that already happens inside of a transaction.

do_the_database work starts a transaction then does:

examine_pms->index_by_files->examine_fio->examine_pkg->give_regdowner_perms
Or
examine_pms->index_by_meta->examine_pkg->give_regdowner_perms
while in the transaction.

lib/PAUSE/Indexer/Context.pm

lib/PAUSE/Indexer/Errors.pm

lib/PAUSE/dist.pm

lib/PAUSE/mldistwatch.pm

wolfsage · 2023-04-30T09:04:41Z

A bunch of code can also be removed if you wish like things referencing INDEX_WARNINGS and has_index_warnings... etc

rjbs · 2023-04-30T10:51:28Z

I reworded the awkward commit. I also removed INDEX_WARNING stuff, which led me to find that the prelude-to-warnings text was not being included.

rjbs · 2024-05-03T21:55:15Z

@wolfsage As discussed today, I think this is reviewable.

wolfsage

checks clock 2h6m. whew.

There's.... a lot here. I've only looked at the code, not played with it. I've left a number of questions.

I think there's at least one legitimate bug which implies more tests could be written, and some code that was either lost in a rebase, or didn't make sense to keep in (lc_package in an UPDATE where it's probably not needed).

I question whether we've introduced mem leaks by not deleting $SELF->{DIO} or FIO everywhere like we used to.

I'd like to play with this and test it on a running inabox, but I cannot do that tonight. Wednesay perhaps if you want to wait more.

Otherwise, seems like good improvements, for the bits I could understand the changes without my brain melting

lib/PAUSE/Indexer/Errors.pm

lib/PAUSE/package.pm

lib/PAUSE/Indexer/Context.pm

wolfsage · 2024-05-06T23:05:19Z

lib/PAUSE/Indexer/Context.pm

+sub add_dist_error {
+  my ($self, $error) = @_;
+
+  $error = ref $error ? $error : { ident => $error, message => $error };


Later code does:

my @dist_errors = $ctx->dist_errors; for my $error (@dist_errors) { my $header = $error->{header}; my $body = $error->{body};

But if you say add_dist_error("foobar"), there will be no header or body. We should probably just drop the string support and always expect the object/hashref

I think you're confused, but maybe I'm confused.

The ternary expression says "if $error was a reference, leave it one. otherwise, turn it into one where it's both ident and message are the string that was $error". So, whether you pass a string or reference to add_dist_error, you get a reference stored in all_dist_errors. The "later code" you cite will get those references and treat them as references.

This doesn't have to do with whether or not later code expects references, it's what that later code expects the references to contain.

As I said, later code wants a header and a body key, and doesn't care at all about ident or message. Here's the full snippet of what later code wants:

my @dist_errors = $ctx->all_dist_errors; for my $error (@dist_errors) { my $header = $error->{header}; my $body = $error->{body}; $body = $body->($self) if ref $body; push @m, "## $header\n\n"; push @m, $tf->format($body), qq{\n\n}; $status_over_all = "Failed"; }

If add_dist_error is called with a string, and we ever somehow do a mail summary send, it seems to me we'll push:

## \n\n

(for header and body respectively, as they'll be undef)
and generate two warnings.

Relatedly, if we ever do send a public summary, it seems like we don't attempt to filter out non-public errors! We should?

Okay, I've fixed this.

wolfsage · 2024-05-06T23:09:52Z

lib/PAUSE/package.pm

@@ -257,21 +241,13 @@ sub examine_pkg {

  # should they be cought earlier? Maybe.
  # but as an ultimate sanity check suggested by Richard Soderberg
-  if ($self->_pkg_name_insane) {
-      $Logger->log("package[$package] name seems illegal");
-      delete $self->{FIO};    # circular reference


We lost a lot of circular reference cleanup in this PR. Is this going to cause leaks?

I don't think so, but it almost doesn't matter: mldistwatch does not make a lot of these, and it doesn't run persistently. Still I'll give it a think.

wolfsage · 2024-05-06T23:16:42Z

lib/PAUSE/package.pm

-        another) workaround against "Safe" limitations.)},
-
-                              );
+          # TODO: get $err->{openerr} back in here, I guess?


Is this todo something to worry about?

I think it's okay to leave like this, but … I dug into this whole hunk of code and HOO BOY. This is a whole 'nother project for a whole 'nother time.

wolfsage · 2024-05-06T23:16:54Z

lib/PAUSE/package.pm

-      delete $self->{FIO};    # circular reference
-      return;
+
+      # TODO: get $err->{line} back in here, I guess?


is this todo something to worry about?

lib/PAUSE/package.pm

This is largley to make the subroutine easier to think about.

The indexer, here, kept doing this: if ($ok) { ... } if ($ok) { ... } ...and any given block might set $ok to 0, but never return early. That meant that it was hard to know whether $ok might get set back later. It never would. Instead, the code now returns early, because $ok can only become true early on, and from then on is really a way of saying "abort, but keep running through code anyway". Better to just return.

This adds the code needed to have the context object track package indexing status. It isn't used yet, but it shows that we need to rename distribution-level errors, so they're unambiguous, and then does that!

Here, we replace all the package-level errors, logged with ->index_status, with ->abort_indexing_package. This lets us put all the error status text in one place and ensure it exists. It also means we can stop worrying about the flow control of package indexing: throw to stop.

In the previous commit, we'd replaced all use of ->index_status to set the current status, and instead used the context methods to record package status. Still, though, those methods set *both* the new status and the old status, and we only read things from the old status. In this commit, we switch to only reading things from the new status, and delete the attributes used to store the old status.

That way, your error can say "this was in previous file X" and not just "in a previous file, not named here".

It just looks better!

I had commented out some code so that we would only check in a package update if we were in an OK state. That is no longer needed, because if we are not in an OK state, we will have thrown an exception. Removing the commented-out code just makes things easier to skim.

We pass "package" and not "pack".

The old code would promote a string to a hashref, but it did it badly. Even when done correctly, the code in abort_indexing_dist would look for $error->{message}, but when passing a DISTERROR result, there was none. This was just general confusion. Now, ->abort_indexing_dist, and thus ->add_dist_error, expect a hashref with, at minimum, a "header" value.

Now that abort_indexing_dist must be passed a hashref, fix all the calling code to do so. This has the side-effect of making all these errors public. That's okay because: 1. we were ignoring "public" anyway! 2. they're all reasonable to show the user

This case doesn't come up yet, and wasn't handled properly. This code will better handle private errors, and also errors with a header and no body.

rjbs · 2024-06-22T15:45:32Z

I've added commits from PAUSE::Indexer::Errors: fix keys from $old to PAUSE::dist: do not include private errors in report which I think address the last(???) outstanding thing. I'm going to look into coverage a bit more.

This makes it easier to see that subs in this file are not part of the top-level package.

Right now, this has no effect, but it will give us a place to put extra diagnostic tooling for collecting mail from test runs.

...then I used this to get all the emails produced by the mldistwatch tests to inspect in my MUA!

rjbs · 2024-06-22T16:28:08Z

I added code to let me get an mbox of all mail sent during testing, which I then inspected in my MUA. Looks pretty good.

rjbs · 2024-06-22T17:14:11Z

I added just a bit more test coverage for a notable but never-called-during-tests method.

rjbs force-pushed the index-context branch 7 times, most recently from ee215ba to b4ec7b6 Compare April 30, 2023 07:40

wolfsage requested changes Apr 30, 2023

View reviewed changes

rjbs force-pushed the index-context branch from b4ec7b6 to 4a1e357 Compare April 30, 2023 08:57

rjbs force-pushed the index-context branch from 4a1e357 to 8be7c64 Compare April 30, 2023 10:50

rjbs force-pushed the index-context branch 4 times, most recently from af03add to ba36b83 Compare May 3, 2023 23:52

rjbs requested a review from wolfsage May 3, 2023 23:53

rjbs marked this pull request as ready for review May 3, 2023 23:53

rjbs mentioned this pull request May 3, 2023

Should PAUSE not index files in eg/ or examples/ ? #226

Open

andk force-pushed the master branch from 15e489b to c14cafc Compare April 25, 2024 12:34

rjbs force-pushed the index-context branch 6 times, most recently from 5c2f1b0 to 39e973c Compare April 28, 2024 11:45

rjbs added the indexer How we index uploads label Apr 28, 2024

rjbs force-pushed the index-context branch from 39e973c to 53c59c3 Compare May 2, 2024 12:41

wolfsage reviewed May 6, 2024

View reviewed changes

rjbs added 20 commits June 22, 2024 10:42

indexer: introduce an Abort::Package catch loop

4905131

indexer: factor our part of the package indexer

8dda205

This is largley to make the subroutine easier to think about.

indexer context: add package-level indexing status

0ce0d54

This adds the code needed to have the context object track package indexing status. It isn't used yet, but it shows that we need to rename distribution-level errors, so they're unambiguous, and then does that!

indexer: clean up a weirdly-formatted comment

584c086

indexer: reintroduce parameters to package-level errors

39bb0b5

That way, your error can say "this was in previous file X" and not just "in a previous file, not named here".

indexer: improve what we provide as overall status

5325b8d

indexer: word wrap the "nothing updated" text

e97b634

It just looks better!

PAUSE::Indexer::Errors: make sure Carp is loaded, we use it!

ce6d391

PAUSE::Package: pass older_isa_regular_perl along

05a0bef

PAUSE::Indexer::Context: make alert and error methods look the same

46dad7b

PAUSE::Indexer::Context: log package errors when setting them

1f56eb6

Mock::Dist: remove this unused code

9d94118

PAUSE::Indexer::Errors: fix keys from $old

7996eec

We pass "package" and not "pack".

PAUSE::dist: do not include private errors in report

6758a60

This case doesn't come up yet, and wasn't handled properly. This code will better handle private errors, and also errors with a header and no body.

rjbs force-pushed the index-context branch from 8387484 to 6758a60 Compare June 22, 2024 15:43

rjbs added 3 commits June 22, 2024 11:55

PAUSE::DBError: use block-style package

4662714

This makes it easier to see that subs in this file are not part of the top-level package.

PAUSE.pm: move email sending into PAUSE.pm

344f6a0

Right now, this has no effect, but it will give us a place to put extra diagnostic tooling for collecting mail from test runs.

PAUSE.pm: add "write mail to mbox" facility

fac7463

...then I used this to get all the emails produced by the mldistwatch tests to inspect in my MUA!

rjbs added 2 commits June 22, 2024 13:12

indexer: add tests for "nothing to index" email

a6e19d1

indexer: be more precise in "nothing to index" email text

3fbb036

rjbs requested a review from wolfsage June 22, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indexing context: make it easier to log and report on indexing status #405

indexing context: make it easier to log and report on indexing status #405

rjbs commented Apr 29, 2023 •

edited

Loading

wolfsage left a comment

wolfsage commented Apr 30, 2023 •

edited

Loading

rjbs commented Apr 30, 2023

rjbs commented May 3, 2024

wolfsage left a comment

wolfsage May 6, 2024

rjbs May 11, 2024

wolfsage May 19, 2024

wolfsage May 19, 2024 •

edited

Loading

rjbs Jun 22, 2024

wolfsage May 6, 2024

rjbs May 7, 2024

wolfsage May 6, 2024

rjbs Jun 22, 2024

wolfsage May 6, 2024

rjbs commented Jun 22, 2024

rjbs commented Jun 22, 2024

rjbs commented Jun 22, 2024

indexing context: make it easier to log and report on indexing status #405

Are you sure you want to change the base?

indexing context: make it easier to log and report on indexing status #405

Conversation

rjbs commented Apr 29, 2023 • edited Loading

wolfsage left a comment

Choose a reason for hiding this comment

wolfsage commented Apr 30, 2023 • edited Loading

rjbs commented Apr 30, 2023

rjbs commented May 3, 2024

wolfsage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfsage May 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbs commented Jun 22, 2024

rjbs commented Jun 22, 2024

rjbs commented Jun 22, 2024

rjbs commented Apr 29, 2023 •

edited

Loading

wolfsage commented Apr 30, 2023 •

edited

Loading

wolfsage May 19, 2024 •

edited

Loading