indexing context: make it easier to log and report on indexing status by rjbs · Pull Request #405 · andk/pause

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

⚠️ This now expects #597 to be merged first.

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.
We no longer repack archives with a+w mode files. We just reject it. (This has come up once in the last 13 years, from a quick look.)
We send failure reports in more cases, but still not trial releases.

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.

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

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

-        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

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


is this todo something to worry 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.

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!

When filter_ppps was simplified, the push of a surviving package ended up inside the loop over rule types. So a package was kept once for each rule type it didn't match, and dropped entirely when there were no package or namespace rules (for example, when no_index had only directory rules, as ExtUtils::MakeMaker writes by default). It was also kept if it passed the package rules but matched a namespace rule. Now we push it once, after checking every rule type. Co-Authored-By: Claude <[email protected]>

examine_pkg passed the string "invalid package name" to abort_indexing_package, which wants an error hashref. Dereferencing the string died, and since that wasn't an Abort::Package exception, it escaped the indexer entirely, ending the whole run. Any dist whose META "provides" named a malformed package could do this. Use the bad_package_name error, which existed for this but was unused. Co-Authored-By: Claude <[email protected]>

When an eval around indexing fails, we rethrow anything that isn't the Abort exception we expect. If $@ was an unblessed reference, calling ->isa on it died with "Can't call method isa on unblessed reference", hiding the real error. Check blessed first. Co-Authored-By: Claude <[email protected]>

When the distribution_type check moved into check_indexability, its "return" stopped skipping the dist and only left check_indexability, so script dists got indexed. Now the check aborts indexing with a new script_dist error. This also changes behavior from before the refactor: previously, script dists were skipped silently, but now the author gets a failure report explaining why nothing was indexed. Co-Authored-By: Claude <[email protected]>

Every dist error was public, so the refactor started sending a failure report for every skipped upload, including all trial releases. Before the refactor, several skips (trial releases, non-dist files, perl-like archives from non-pumpkings, untar failures) sent no report at all. Trial releases go back to being silent. The others now send a report, which is a deliberate change: the uploader should find out why nothing was indexed. Later, we may want a gentler "declined" report for some of these, rather than "Failed". Co-Authored-By: Claude <[email protected]>

Nothing used these anymore.

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

Comment thread lib/PAUSE/Indexer/Context.pm Outdated

Comment thread lib/PAUSE/Indexer/Errors.pm

Comment thread lib/PAUSE/Indexer/Errors.pm

Comment thread lib/PAUSE/dist.pm

Comment thread lib/PAUSE/mldistwatch.pm Outdated

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 and others added 29 commits October 12, 2026 09:59

indexer context: add package-level indexing status

c4d6d25

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

5a8cf75

indexer: reintroduce parameters to package-level errors

c5a226e

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

cb5027b

indexer: word wrap the "nothing updated" text

a54a5e4

It just looks better!

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

097d6c5

PAUSE::Package: pass older_isa_regular_perl along

f3e7788

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

f8f1184

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

3612809

Mock::Dist: remove this unused code

a3102d9

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

40521fa

We pass "package" and not "pack".

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

7aa2692

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.

PAUSE::DBError: use block-style package

bb2f4a3

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

013d5f6

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

4e5f8c9

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

indexer: add tests for "nothing to index" email

1c9dfc0

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

36f48df

PAUSE::package: drop dead args to __do_regular_perl_update

3a34b0c

Nothing used these anymore.

rjbs force-pushed the index-context branch from 239db4a to 3a34b0c Compare October 12, 2026 13:59

Conversation

rjbs commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfsage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfsage commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjbs commented Apr 30, 2023

Uh oh!

rjbs commented May 3, 2024

Uh oh!

wolfsage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfsage May 6, 2024

Choose a reason for hiding this comment

Uh oh!

rjbs Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

wolfsage May 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rjbs commented Apr 29, 2023 •

edited

Loading

wolfsage commented Apr 30, 2023 •

edited

Loading