Opened 5 weeks ago

Last modified 4 weeks ago

#36010 assigned Bug

compilemessages reports errors only once

Reported by: Balazs Endresz Owned by: Claude Paroz
Component: Internationalization Version: dev
Severity: Normal Keywords:
Cc: Balazs Endresz Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When calling compilemessages for a locale that has invalid entries it prints out the errors correctly first.
But any subsequent calls just say that it's already compiled and up to date.
This is because msgfmt updates the last modified date of the mo file even if it's invalid, and then django will skip that locale afterwards:
https://github.com/django/django/blob/main/django/core/management/commands/compilemessages.py#L153

This can reproduced by calling compilemessages twice in django/tests/i18n/test_compilation.py:

class CompilationErrorHandling(MessageCompilationTests):
    def test_error_reported_by_msgfmt(self):
        # po file contains wrong po formatting.
        with self.assertRaises(CommandError):
            call_command("compilemessages", locale=["ja"], verbosity=0)

        # subsequent calls should still fail
        with self.assertRaises(CommandError):
            call_command("compilemessages", locale=["ja"], verbosity=0)  # currently this doesn't raise CommandError

One way of fixing this is perhaps doing the validation separately first, e.g. with mo_path = "NUL" if os.name == "nt" else "/dev/null" and then calling msgfmt again with the actual mo_path only if that passes.

Change History (9)

comment:1 by Claude Paroz, 5 weeks ago

Triage Stage: UnreviewedAccepted
Version: 5.1dev

Good catch! I think your suggested resolution strategy makes sense.

comment:2 by amansharma612, 5 weeks ago

Owner: set to amansharma612
Status: newassigned

comment:3 by amansharma612, 4 weeks ago

I don't think that the error is due to msgfmt updating the last modified date, as I found that to not be the case when reproducing the bug.

The following is mentioned in the comments for is_writable

    # Known side effect: updating file access/modified time to current time if
    # it is writable.

Checking is_writable on mo_path modifies the date. Nonetheless, the suggested fix seems reasonable, or there could be a way to implement is_writable without modifying the date.

Last edited 4 weeks ago by amansharma612 (previous) (diff)

comment:4 by Claude Paroz, 4 weeks ago

I cannot remember if I didn't use os.access(file_path, os.W_OK) on purpose or if I didn't know that method at the time. To be tested.

comment:5 by amansharma612, 4 weeks ago

os.open(mo_path, "a") has the semantics of creating a new file as long as the path is accessible (if accessible, is_writable never returns false). In such a scenario, using os.access leads to change in semantics and failing test cases.

I will proceed with the first fix mentioned above to avoid breaking that behaviour.

Version 0, edited 4 weeks ago by amansharma612 (next)

comment:6 by Balazs Endresz, 4 weeks ago

But if we need to run msgfmt regardless of the last modified date then doesn't that make the optimisation to skip it based on the last modified date pointless? So I think that optimisation just has to be removed and always run msgfmt with the same args as we do now. I think that would be the behaviour most people would expect anyway.

comment:7 by amansharma612, 4 weeks ago

Makes sense. I have opened a PR for the same here.

comment:8 by Claude Paroz, 4 weeks ago

Note that your patch is reverting an optimization done to fix #31692.

I would vote for trying a different method to test writability, maybe by writing a temporary file in the parent directory?

I experimented that approach in that PR.

comment:9 by Claude Paroz, 4 weeks ago

Has patch: set
Owner: changed from amansharma612 to Claude Paroz
Note: See TracTickets for help on using tickets.
Back to Top