Repository navigation
Conversation
Owner
|
@rjbs, could you please review and fix the conflicts? |
rjbs
force-pushed
the
very-small-version
branch
from
April 30, 2023 07:59
f2c3ee5 to
4194459
Compare
rjbs
marked this pull request as draft
April 30, 2023 07:59
rjbs
force-pushed
the
very-small-version
branch
from
April 30, 2023 08:57
4194459 to
f492747
Compare
rjbs
force-pushed
the
very-small-version
branch
3 times, most recently
from
April 28, 2024 11:45
51027c5 to
0774909
Compare
Before, the call sequence was something like:
give_regdowner_perms ❗️
case check
version error check
final sanity check
checkin ❗️
...where the ❗️-marked lines could write to the database. This had the
effect that if the code between them returned early, the package would
have caused partial updates to the database, adding new package
permissions without indexing the package.
While we probably want to combine these in a transaction or savepoint,
this commit does the simple thing: moves the first
(give_regdowner_perms) into the other (checkin), so they run together,
with no opportunity to exit early. This should make it safer to abort
during package indexing.
The plan will be to use this to gather indexing state, to trigger abords, and to decide how to report the result to the user.
This lets us easily register errors with name and description, and to use those errors to easily say "stop the whole dist for this reason" and have the right thing happen. Subsequent commits will put these new libraries to use.
We replace $dist->{SKIP} with $ctx->dist_errors. This accumulates a
list of reasons to stop indexing the whole dist (although usually only
one can show up).
$dist->{REASON_TO_SKIP} is also replaced by dist_errors. Previously,
SKIP (a bool) meant to skip and REASON_TO_SKIP meant "and tell the user
this". Now, there is always a reason, and the error contains a separate
flag indicating whether to send it to the user.
Previously, some skip reasons were REASON_TO_SKIP, but others were
other properties on the $dist object, like HAS_BLIB or
HAS_MULTIPLE_ROOT. These are now dist errors, so there's a uniform
handling of their description, and they can abort processing in a
uniform way.
Code that wants to abort can $ctx->abort_indexing, which adds a
dist_error and uses an exception to stop processing the dist further.
I deleted a bunch of code managing the "you don't have permissions on
the distribution-name package", which is the thing that asserts you must
have permission on Foo::Bar to upload Foo-Bar. Previously, this code
still summarized each pmfile scanned, and then fixed up the results to
act like passing modules failed. Now, this error is simply displayed as
a total dist failure. I think it should be possible to show the package
indexing summary again in the future, but for now the code was overly
complex and didn't really add much value.
I broke out two hunks of email content generating code, done just to
reduce the total size of the mail_summary subroutine.
I also eliminated the code that would produce
Your-Tarball-withoutworldwriteables, which was meant to automatically
correct for authors uploading archives with a+w permissions on their
files. In the last 13 years, this seems to have rescued only one
still-indexed dist. It's just an error now.
...by bringing back text I had deleted.
This should be functionally the same, but a bit easier to read and reason about.
This is in preparation for using Abort::Package to abort packge indexing.
This is a simple refactoring, where we pile up the PAUSE::package objects and then loop over them to index. The point of this change is to give us a very small piece of code around which to build the eval/next loop that catches Abort::Package exceptions.
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!
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Nothing used these anymore.
Each package error's body checks for the arguments it needs with _assert_args_present, passing its own name so the failure message can say which error was missing them. Five of them passed "db_conflict" instead, so a missing argument for, say, version_fell would be reported as a problem with db_conflict. Co-Authored-By: Claude <noreply@anthropic.com>
If the author writes a bare numeric literal as a version, like $VERSION = 0.000001, perl turns it into 1e-06, which can't become a version object. Before, that died inside normalize_version, examine_fio logged "error with version", and the file was silently skipped: the author got no explanation. Now normalize_version returns a JSON error blob for a version that looks like scientific notation, the same way parse_version does for other version problems, and examine_pkg maps that blob to a new package error, version_numeric_literal, so the indexer report tells the author to quote the version. This was originally written against a much older indexer, where getting the message up the call stack was impractical and the test for the report was a TODO. Rebased onto the index-context refactor, the error machinery makes it simple, so the TODO is now a real assertion. Co-Authored-By: Claude <noreply@anthropic.com>
objToJson and jsonToObj are JSON 1.x compatibility names, and JSON 4.x warns on every call that they "will be obsoleted". The numeric-literal version test now exercises both the encode and decode sides in the parent process, so the warnings showed up in test output. Co-Authored-By: Claude <noreply@anthropic.com>
rjbs
force-pushed
the
very-small-version
branch
from
October 22, 2026 00:57
98c5097 to
f08967a
Compare
Collaborator
Author
|
This is now rebased on #405 to be merged soon, I hope. |
rjbs
marked this pull request as ready for review
October 22, 2026 00:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Really, this should become a useful user message, but it's painfully difficult to get that error up through the call stack. This code needs (in my opinion) to be rearchitected to make this sort of thing simple.
In the meantime, the code now detects this case specifically, so we can do something about it later.
Meant to help with #192, but won't help enough until we can get the message passed upward.