gh-130379: Fix incorrect zipapp logic to avoid including the target in itself

[pfmoore]

The change in #106076 was intended to ensure that zipapp would not include the target file in the list of files that get included in the target. However the logic to do that was flawed, and expensive. This PR changes the approach to generate the list of files to add before creating the target, so there is no possibility of the target being included in the first place.

• Issue: Zipapp archives are always empty #130379

[pfmoore]

@serhiy-storchaka you raised some concerns about the original change (unfortunately I'd merged before I saw them). Would you be willing to take a look at this revised approach?

[serhiy-storchaka]

LGTM. This addresses also my concerns.

[pfmoore]

One possible improvement would be to check explicitly for target:

files_to_add = sorted(f for f in source.rglob('*') if f != target)

This would catch the case where the user does something like create_archive(".", target="out.pyz") when there's an existing out.pyz left over from a previous run. Arguably that's a user error (the user should be specifying a filter that ensures unexpected files like this are excluded, if they aren't able to control the content of the source directory) but it does seem like it's a common mistake.

The problem with this check is that it's not complete. A path equality check doesn't cover all cases (symlinks, use of .., etc.). I would prefer to argue that those cases are clearly not simple accidents, and anyone concerned about these should be writing their code more robustly, either tightly controlling the source directory, or using a filter that is tailored to do what they want. Otherwise, we would need to do something costly like check Path.samefile when target is a path. And we'd still have problems if the target was a file-like object writing to a file in the source directory 🙁

I'm inclined to add the simple path equality check, with a comment explaining that it's not intended to catch every case where the target is part of the source, just to act as a sanity check for this specific common error. Does that seem like a reasonable compromise?

[pfmoore]

Report an error if the target file overwrites a file in the list of source files to add. See #130379 (comment) for details.

[serhiy-storchaka]

target can be a file-like object. target in files_to_add can invoke comparison of a file-like object with the Path objects. Is it what we need?

[pfmoore]

I believe it's fine. A file-like object will simply test as not equal to all the Path objects. I guess I could add some sort of exception handling in case the user passes an object with a custom equality check that fails, but that seems like overkill to me.

I was going to add a note to that effect in the comment, but it's long enough already and I wasn't sure it was worth it. The fact that you pointed it out suggests that it would be, though.

[serhiy-storchaka]

I do not think that zipfile should catch this.

[pfmoore]

OK. I'll remove that part of the comment. I think the same logic applies here - if adding the check can't be justified in zipfile, it's not worth adding it here either. We can stick with the simple but incomplete check.

[pfmoore]

@Yhg1s IMO this is a bug fix that should probably be backported to 3.13. The original bug was introduced in #106076, which isn't present in 3.12, so there's no need to backport further. It's technically a behaviour change (the original intent was to skip the offending file rather than error) but given that the intended behaviour didn't work, I think it's better to backport than leave the bug in 3.13.

Before I add the backport label, do you have any objections?

[pfmoore]

I'm going to request a backport. If anyone has any objections, or feels this shouldn't be backported, speak up - it's easy enough to revert if necessary.

[miss-islington-app]

Thanks @pfmoore for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-130791 is a backport of this pull request to the 3.13 branch.