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)
