Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by address@hidden)

[hanwenn]

Reviewers: hahnjo,

Message:
I think this is worth it because it simplifies the build system, and
puts the locking in the place where we actually access the resource.

I take your point about dropped files; the best would be fcntl locks,
but I am worried that they might not be supported on windows. Would you
know?

Maybe we can just use fcntl locks on unix, and Windows users should just
not try to run parallel lp-book invocations.


https://codereview.appspot.com/555360043/diff/551490043/scripts/lilypond-book.py
File scripts/lilypond-book.py (right):

https://codereview.appspot.com/555360043/diff/551490043/scripts/lilypond-book.py#newcode458
scripts/lilypond-book.py:458: lock_file =
os.path.join(options.lily_output_dir + ".lock")
On 2020/02/23 15:18:26, hahnjo wrote:
> At first glance I thought this was wrong and should have two arguments
to join
> (otherwise the call is useless). After seeing that you only mkdir
> lily_output_dir below, maybe you want a file $(basename
lily_output_dir).lock at
> the parent directory? That's a bold assumption that there is no / at
the end...

leftover; removed.

https://codereview.appspot.com/555360043/diff/551490043/scripts/lilypond-book.py#newcode460
scripts/lilypond-book.py:460: while 1:
On 2020/02/23 15:18:26, hahnjo wrote:
> while True

Done.

https://codereview.appspot.com/555360043/diff/551490043/scripts/lilypond-book.py#newcode477
scripts/lilypond-book.py:477: os.close(lockfd)
On 2020/02/23 15:18:26, hahnjo wrote:
> You should close the file before removing it.

on the contrary. We don't have to close it at all  (on exit, all files
are closed automatically.). If we close first, there is a larger chance
of leaving the lock file hanging around.

Description:
Add a cooperative FS lock to lilypond-book.

This simplifies the build infrastructure, because it obviates Makefile
hacks to force a single lilypond-book processes during the build

Please review this at https://codereview.appspot.com/555360043/

Affected files (+25, -11 lines):
  M make/ly-rules.make
  M scripts/lilypond-book.py


Index: make/ly-rules.make
diff --git a/make/ly-rules.make b/make/ly-rules.make
index 
5a0ceaa397d85f37ee10ca0d536e02f6429b5cda..5df0de7f579c5159063e07d0ba6b69a013aaf39c
 100644
--- a/make/ly-rules.make
+++ b/make/ly-rules.make
@@ -14,15 +14,6 @@ $(outdir)/%.latex: %.doc $(INIT_LY_SOURCES) $(SCHEME_SOURCES)
                --output=$(outdir) $(LILYPOND_BOOK_FLAGS) \
                --redirect-lilypond-output $<
 
-
-# This allows -j make option while making sure only one lilypond-book instance
-# is running at the same time, using GNU make's order-only prerequisites so
-# as to not create superficial dependencies between unrelated manuals.
-define CHAIN_RULE
-| $(i)
-$(i): 
-endef
-
 $(eval $(firstword $(TEXI_FILES_FROM_TELY)):\
  $(foreach i, $(wordlist 2, $(words $(TEXI_FILES_FROM_TELY)),\
  $(TEXI_FILES_FROM_TELY)),$(CHAIN_RULE)))
Index: scripts/lilypond-book.py
diff --git a/scripts/lilypond-book.py b/scripts/lilypond-book.py
index 
721dde16553394af9964d61a0f600a0bdd3512d8..71cbbe5db2c8acdbd61eca4c1d31a1a3936d92db
 100644
--- a/scripts/lilypond-book.py
+++ b/scripts/lilypond-book.py
@@ -53,6 +53,7 @@ import re
 import stat
 import sys
 import tempfile
+import time
 from optparse import OptionGroup
 from functools import reduce
 
@@ -453,6 +454,30 @@ def split_output_files(directory):
     return set (files)
 
 def do_process_cmd (chunks, input_name, options):
+    """Wrap do_process_cmd_locked in a filesystem lock"""
+    lock_file = os.path.join(options.lily_output_dir + ".lock")
+    try:
+        while 1:
+            try:
+                lockfd = os.open(lock_file, os.O_CREAT | os.O_EXCL, 0o600)
+                break
+            except FileExistsError:
+                # This is not elegant, but given the startup time of
+                # LilyPond (which is ~1 second), this gives enough
+                # granularity
+                time.sleep(0.05)
+        if not os.path.isdir (options.lily_output_dir):
+            os.makedirs (options.lily_output_dir)
+        do_process_cmd_locked (chunks, input_name, options)
+    finally:
+        try:
+            os.remove(lock_file)
+        except:
+            pass
+        os.close(lockfd)
+
+def do_process_cmd_locked (chunks, input_name, options):
+    """Look at all snippets, write the outdated ones, and compile them."""
     snippets = [c for c in chunks if isinstance (c, 
BookSnippet.LilypondSnippet)]
 
     output_files = split_output_files (options.lily_output_dir)
@@ -739,8 +764,6 @@ def main ():
 
     if global_options.lily_output_dir:
         global_options.lily_output_dir = 
os.path.abspath(global_options.lily_output_dir)
-        if not os.path.isdir (global_options.lily_output_dir):
-            os.makedirs (global_options.lily_output_dir)
     else:
         global_options.lily_output_dir = 
os.path.abspath(global_options.output_dir)