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.

  1. It happens because inside handle(), the write_po_file() method is called in a for loop for every potfile:

https://github.com/django/django/blob/main/django/core/management/commands/makemessages.py#L459

  1. potfiles is a list, 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

  1. 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

  1. Later, it is possible that inside the find_files(), the same locale path is inserted to self.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 Michał Pokusa, 4 months ago

Easy pickings: set

comment:2 by Natalia Bidart, 4 months ago

Resolution: needsinfo
Status: newclosed

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).

comment:3 by Michał Pokusa, 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 Michał Pokusa, 4 months ago

Resolution: needsinfo
Status: closednew

comment:5 by Senthil Kumar, 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

Last edited 4 months ago by Senthil Kumar (previous) (diff)

in reply to:  5 comment:6 by Michał Pokusa, 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".

Last edited 4 months ago by Michał Pokusa (previous) (diff)

comment:7 by Jacob Walls, 4 months ago

Owner: set to Michał Pokusa
Status: newassigned

comment:8 by Natalia Bidart, 4 months ago

Cc: Claude Paroz added
Easy pickings: unset
Triage Stage: UnreviewedAccepted

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 Michał Pokusa, 4 months ago

Has patch: set

comment:10 by Claude Paroz, 4 months ago

Needs tests: set

comment:11 by Michał Pokusa, 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?

in reply to:  11 comment:12 by Natalia Bidart, 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 Michał Pokusa, 3 months ago

Needs tests: unset

I have added tests that check for duplicate locale paths and write_po_file calls.

comment:14 by kay, 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 Paths 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 Sarah Boyce, 4 weeks ago

Needs tests: set

comment:16 by Michał Pokusa, 4 weeks ago

Needs tests: unset

comment:17 by Sarah Boyce, 3 weeks ago

Triage Stage: AcceptedReady for checkin

comment:18 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 2c99fbc:

Fixed #36368 -- Prevented duplicate locale paths and write_po_file calls in makemessages.

Note: See TracTickets for help on using tickets.
Back to Top