1750281 - [compare-locales] Concurrent invocations of `compare-locales` race to create parent directories

Status: RESOLVED FIXED

(Localization Infrastructure and Tools :: compare-locales, enhancement)

Description

[Nick Alexander :nalexander [he/him]]

The FasterMake/artifact build system invokes compare-locales via l10n_merge.py. Concurrent invocations can race to create parent directories, and this cannot be fixed in tree because compare-locales is vendored from upstream: see

task 2022-01-14T19:19:56.920Z]  2:25.08 python/mozbuild/mozbuild/test/test_vendor.py::test_up_to_date_vendor TEST-UNEXPECTED-FAIL
...

like https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=GFY6vX_2SrKIZO2KntSTTg.0&revision=cb033852bab5116a7772584ff6fae5d9d96bd1ef.

This ticket tracks addressing this with a patch like:

# HG changeset patch
# User Nick Alexander <[e-mail]>
# Date 1625510634 25200
#      Mon Jul 05 11:43:54 2021 -0700
# Node ID b0e8f050a7ee2a91616e161b1ba2998741504eb1
# Parent  82bf2085ae3474d32848ce00ec9e06028267683f
Bug 1358824 - Pre: Fix race making directories when comparing locales.

There's a time-of-check vs time-of-use race here that is harmless save
that it raises an error when multiple locales are built concurrently.

diff --git a/third_party/python/compare_locales/compare_locales/compare/content.py b/third_party/python/compare_locales/compare_locales/compare/content.py
--- a/third_party/python/compare_locales/compare_locales/compare/content.py
+++ b/third_party/python/compare_locales/compare_locales/compare/content.py
@@ -34,8 +34,7 @@ class ContentComparer:
 
     def create_merge_dir(self, merge_file):
         outdir = mozpath.dirname(merge_file)
-        if not os.path.isdir(outdir):
-            os.makedirs(outdir)
+        os.makedirs(outdir, exist_ok=True)
 
     def merge(self, ref_entities, ref_file, l10n_file, merge_file,
               missing, skips, ctx, capabilities, encoding):

Comment 1

[Nick Alexander :nalexander [he/him]]

flod: are you the right person to fix this tiny issue? Or should I figure out how to submit a fix to compare_locales upstream?

Comment 2

[Francesco Lodolo [:flod]]

Definitely not the right person, redirecting to Eemeli.

Code now lives here https://github.com/mozilla/compare-locales

Comment 3

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #2)

Definitely not the right person

To clarify comment left in a hurry: I'm a good first point of contact for localization related questions, and I can redirect as needed.
I'm just not good for code review, especially in this case ;-)

Comment 4

[Eemeli Aro [:eemeli]]

The fix looks entirely appropriate, and does the right thing. So I applied it to the now-git-hosted compare-locales repo and released compare-locales@8.2.1 including it: https://pypi.org/project/compare-locales/8.2.1/

Nick, were you planning on updating the vendored copy in m-c yourself, or would you like me to do so?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Eemeli Aro [:eemeli] from comment #4)

The fix looks entirely appropriate, and does the right thing. So I applied it to the now-git-hosted compare-locales repo and released compare-locales@8.2.1 including it: https://pypi.org/project/compare-locales/8.2.1/

Nick, were you planning on updating the vendored copy in m-c yourself, or would you like me to do so?

Would you please update it? Thanks!

Comment 6

[Eemeli Aro [:eemeli]]

Bother, it looks like I can't, at least easily atm. mach vendor python requires Python 3.6, and that's not supported on my M1 MacBook. Naively installing 3.6.15 via pyenv fails, and I'm not convinced that jumping through the hoops that the Internet recommends to make it work are necessarily all good ideas.

Nick, would you have access to a suitable python version? AFAICT, this should be a simple case of updating third_party/python/requirements.in with compare-locales==8.2.1 and then letting mach do its thing.

Comment 7

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Bug 1750281: Bump `compare-locales` from 8.1.0 to 8.2.1

Resolves race condition on creating parent directories when
run in parallel.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d6e8ea02e71
Bump `compare-locales` from 8.1.0 to 8.2.1 r=eemeli

Comment 9

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/5d6e8ea02e71