Repository navigation
Conversation
rjbs
marked this pull request as draft
May 12, 2024 01:27
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!
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 <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>
Upload the simplest possible dist as both a tarball and a zip file, and check that each is indexed the same way. This needed Module::Faker 0.027, which chmods the files it puts in a zip archive to 0644; before that, Archive::Any::Create made them world writable and PAUSE rejected the upload.
p6_index_dist lists the archive with tar to find bin/ entries. For a zip archive that fails, and the failure aborts indexing the whole dist. Pick the listing command by file extension; "unzip -Z1" prints one entry name per line, the same shape as "tar tf". Co-Authored-By: Claude <noreply@anthropic.com>
untar sets PERL_MAJOR_VERSION while listing the tarball, but the zip branch of examine_dist never set it, so perl_major_version was undef for every zip upload. That meant "uninitialized value" warnings on each comparison and, for an actual Perl 6 dist in a zip, indexing it as a Perl 5 dist with no META file. After unzipping, look for a top-level-directory META6.json, the same test untar applies to the listing. Co-Authored-By: Claude <noreply@anthropic.com>
corpus/mld/perl6-zip holds the same Inline-1.1 dist as corpus/mld/perl6, repacked as a zip file, and the Perl 6 indexing test now runs over both. Co-Authored-By: Claude <noreply@anthropic.com>
Each insert pushed (errstr, ret) onto @Args but logged the first as "ret" and the second as "err", so a successful insert was logged as ret "" and err 1. Push them in the order the log reads them. Co-Authored-By: Claude <noreply@anthropic.com>
rjbs
force-pushed
the
archive-zip-tests
branch
from
October 22, 2026 01:15
84592b6 to
111b93a
Compare
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.
This is blocked by a new Module-Faker, which is kinda waiting on miyagawa/Archive-Any-Create#3
Also, this is really just one commit on top of #510
Also, this exposes that
->untarsetsPERL_MAJOR_VERSIONbut zip archives never have it set. Other discrepancies may show up, too.