Opened 4 months ago
Closed 3 weeks ago
#36368 closed Bug (fixed)
`makemessages` command runs `write_po_file()` multiple times for the same potfile
Reported by: | Michał Pokusa | Owned by: | Michał Pokusa |
---|---|---|---|
Component: | Core (Management commands) | Version: | 5.2 |
Severity: | Normal | Keywords: | makemessages write_po_file |
Cc: | Claude Paroz | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This is both a Bug and a Cleanup/optimization issue.
In some cases, the makemessages
management command will run write_po_file()
multiple times per the same potfile and locale.
- It happens because inside
handle()
, thewrite_po_file()
method is called in a for loop for everypotfile
:
https://github.com/django/django/blob/main/django/core/management/commands/makemessages.py#L459
potfiles
is alist
, and that makes it possible for it to have duplicates, which is exactly the case when the bug described here happens.
Inside build_potfiles()
, used for potfiles
inside handle()
, inside a for loop for every path in self.locale_paths
, the returned list is appended.
https://github.com/django/django/blob/main/django/core/management/commands/makemessages.py#L513
- The
self.locale_paths
is also a list, and it also can have duplicates, because e.g. when the<project root>/locale
folder is present, it is being appended twice:
https://github.com/django/django/blob/main/django/core/management/commands/makemessages.py#L384
- Later, it is possible that inside the
find_files()
, the same locale path is inserted toself.locale_paths
once again, which makes it possible to have 2 duplicates or 3 in total of the same locale path.
I propose the solution to change self.locale_paths
from list
to set
, and do the same for potfiles
inside handle()
, after doing that the problems seems to be fixed. This also shouldn't break anything, because it looks like this is how it is supposed to work, meaning ony one occurence of each value.
If the issue is accepted I would be happy to be assigned to it and make a PR.
Change History (18)
comment:1 by , 4 months ago
Easy pickings: | set |
---|
comment:2 by , 4 months ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:3 by , 4 months ago
Thank you for looking into it.
I prepared a very minimal project that will show the issue:
https://github.com/michalpokusa/django-test-project/tree/ticket-36368
Everything is default as from the startproject
except the optional LOCALE_PATHS
setting and a debug_makemessages
command, which is copy-pasted makemessages
command with some prints for clarity.
The bug is present also in makemessages
, but it is not clearly visible, that is why I added this command, but it is not necessary to reproduce.
Without setting LOCALE_PATHS
in settings
:
(.venv) [2025-05-06 00:44:31] michalpokusa@dev:~/projects/django-ticket-xyz$ python manage.py debug_makemessages --locale en settings.LOCALE_PATHS: [] locale_paths inside handle(): ['/home/michalpokusa/projects/django-ticket-xyz/locale'] self.locale_paths at the end of find_files(): ['/home/michalpokusa/projects/django-ticket-xyz/locale', '/home/michalpokusa/projects/django-ticket-xyz/locale'] potfiles returned from build_potfiles(): ['/home/michalpokusa/projects/django-ticket-xyz/locale/django.pot', '/home/michalpokusa/projects/django-ticket-xyz/locale/django.pot'] processing locale en calling write_po_file calling write_po_file
With LOCALE_PATHS
set to [BASE_DIR / "locale"]
:
(.venv) [2025-05-06 00:48:44] michalpokusa@dev:~/projects/django-ticket-xyz$ python manage.py debug_makemessages --locale en settings.LOCALE_PATHS: [PosixPath('/home/michalpokusa/projects/django-ticket-xyz/locale')] locale_paths inside handle(): [PosixPath('/home/michalpokusa/projects/django-ticket-xyz/locale'), '/home/michalpokusa/projects/django-ticket-xyz/locale'] self.locale_paths at the end of find_files(): ['/home/michalpokusa/projects/django-ticket-xyz/locale', PosixPath('/home/michalpokusa/projects/django-ticket-xyz/locale'), '/home/michalpokusa/projects/django-ticket-xyz/locale'] potfiles returned from build_potfiles(): ['/home/michalpokusa/projects/django-ticket-xyz/locale/django.pot', '/home/michalpokusa/projects/django-ticket-xyz/locale/django.pot', '/home/michalpokusa/projects/django-ticket-xyz/locale/django.pot'] processing locale en calling write_po_file calling write_po_file calling write_po_file
comment:4 by , 4 months ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
follow-up: 6 comment:5 by , 4 months ago
Hey, I'm new to the community and would love to try troubleshooting and proposing a solution. May I take this on? Thanks
comment:6 by , 4 months ago
Replying to Senthil Kumar:
Hey, I'm new to the community and would love to try troubleshooting and proposing a solution. May I take this on? Thanks
Hi, thank you for taking interest in this issue, although I think there is nothing more to troubleshoot here, as both the direct cause and the solution are determined.
Right now I am waiting with PR for the issue to be accepted and information whether anything else should be included in it.
EDIT:
I see that right now this issue is literally the only one "Easy picking" that is "new".
comment:7 by , 4 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:8 by , 4 months ago
Cc: | added |
---|---|
Easy pickings: | unset |
Triage Stage: | Unreviewed → Accepted |
Thank you Michał Pokusa for the test project. I have used that as starting point, and I can confirm that there are two places where the files list to be processed could have duplicates included. One is where you said:
if self.settings_available: self.locale_paths.extend(settings.LOCALE_PATHS)
But also here, and dupes can be seen when running for example i18n.test_extraction.BasicExtractorTests.test_valid_locale
(locale_paths
end up containing, for example, ['/tmp/django__fn2ga0m/i18n_zmigwa3d/commands/locale', '/tmp/django__fn2ga0m/i18n_zmigwa3d/commands/locale']
):
elif dirname == "locale": dirnames.remove(dirname) self.locale_paths.insert( 0, os.path.join(os.path.abspath(dirpath), dirname) )
I'm accepting with the goal that we handle duplicates in a way that does not alter the order of the locale folders being processed. We need to respect the order described in these docs.
comment:9 by , 4 months ago
Has patch: | set |
---|
comment:10 by , 4 months ago
Needs tests: | set |
---|
follow-up: 12 comment:11 by , 4 months ago
I have prepared a PR with fix with respect to the order described in docs and added appropriate tests.
Is there anything else that needs to be done on my part or should I now wait for the review on Github?
comment:12 by , 3 months ago
Replying to Michał Pokusa:
I have prepared a PR with fix with respect to the order described in docs and added appropriate tests.
Thank you!
Is there anything else that needs to be done on my part or should I now wait for the review on Github?
If you consider the PR ready to be reviewed again, you should ideally update this ticket's flag so the PR is listed as "needing review" in the Django Development Dashboard.
In this specific case, it means that you should unset the "needs test" flag which was set by a previous reviewer. More docs here: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-review-checklist
comment:13 by , 3 months ago
Needs tests: | unset |
---|
I have added tests that check for duplicate locale paths and write_po_file
calls.
comment:14 by , 7 weeks ago
I came across this bug just this week!
My fix (for the portion that compares dirname
) would have looked almost identical, except I named my variable locale_dir_path
instead of locale_dir
to be extra verbose about its contents (vs. dirname
's) and opted to only insert
matches not
already in self.locale_paths
anyway rather than removing and (re)inserting them.
Good thinking re: the potential for LOCALE_PATHS
containing Path
s rather than strings, Michał, even if that part may have needed to go in a separate ticket as slightly different (if related) concern.
Is there anything regular users can do to speed up contributions by others?
comment:15 by , 4 weeks ago
Needs tests: | set |
---|
comment:16 by , 4 weeks ago
Needs tests: | unset |
---|
comment:17 by , 3 weeks ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hello Michał Pokusa, thank you for your ticket! I think I followed your explanation, though I don't see how
self.locale_paths
could have duplicates. If you could show in a Django sample project or in a unit test how this scenario can be reached, I'll be happy to accept this ticket.I'll close as
needsinfo
for now but please reopen when you can provide further details (i.e. a real case scenario for having duplicates in the path lists).