[Django] #36010: compilemessages reports errors only once

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Type: Bug
Status: new | Component:
| Internationalization
Version: 5.1 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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`:

{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36010>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36010: compilemessages reports errors only once

--------------------------------------+------------------------------------
Reporter: Balazs Endresz | Owner: (none)
Type: Bug | Status: new
Component: Internationalization | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Claude Paroz):

* stage: Unreviewed => Accepted
* version: 5.1 => dev

Comment:

Good catch! I think your suggested resolution strategy makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:1>

[Django]

#36010: compilemessages reports errors only once

-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by amansharma612):

* owner: (none) => amansharma612
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:2>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by amansharma612):

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
[https://github.com/django/django/blob/3ee4c6a27ad520d4ecc3cded260d47cbccafb144/django/core/management/commands/compilemessages.py#L20
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:3>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:4>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by amansharma612):

`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:5>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Balazs Endresz):

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:6>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by amansharma612):

Makes sense. I have opened a PR for the same
[https://github.com/django/django/pull/18955 here].
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:7>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner:
| amansharma612
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

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
[https://github.com/django/django/pull/18957 that PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:8>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------

Reporter: Balazs Endresz | Owner: Claude
| Paroz

Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Claude Paroz):

* has_patch: 0 => 1
* owner: amansharma612 => Claude Paroz

--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:9>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner: Claude
| Paroz
Type: Bug | Status: assigned
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:10>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner: Claude
| Paroz

Type: Bug | Status: closed
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"2c47207b3c8412d16e61e388f176b47b41b40794" 2c47207]:
{{{#!CommitTicketReference repository=""
revision="2c47207b3c8412d16e61e388f176b47b41b40794"
Fixed #36010 -- Avoided touching mo files while checking writability.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:11>

[Django]

#36010: compilemessages reports errors only once
-------------------------------------+-------------------------------------
Reporter: Balazs Endresz | Owner: Claude
| Paroz
Type: Bug | Status: closed
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"72c0359dda80df22d0e922874ad0377e20f90be7" 72c0359]:
{{{#!CommitTicketReference repository=""
revision="72c0359dda80df22d0e922874ad0377e20f90be7"
[5.2.x] Fixed #36010 -- Avoided touching mo files while checking
writability.

Backport of 2c47207b3c8412d16e61e388f176b47b41b40794 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36010#comment:12>