Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(757)

Issue 561810045: Prevent race condition in `-dfont-ps-resdir`

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by trueroad
Modified:
4 months, 3 weeks ago
Reviewers:
lemzwerg
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Prevent race condition in `-dfont-ps-resdir` Issue 5968/4: Add Windows support for `-dfont-ps-resdir` Windows environment could not use `-dfont-ps-resdir` option because it used symlink. This commit makes using file copying instead symlink when Windows. Issue 5968/3: Prevent creating file race condition in `-dfont-ps-resdir` For `-dfont-ps-resdir`, we used creating file after checking the file existence. However, when multiple lilypond processes were running, as when using lilypond-book, a race condition could cause creating file to fail. This commit uses creating file method for `-dfont-export-dir` that can prevent race condition. Issue 5968/2: Prevent symlink race condition in `-dfont-ps-resdir` For `-dfont-ps-resdir`, we used symlink after checking the file existence. However, when multiple lilypond processes were running, as when using lilypond-book, a race condition could cause symlink to fail. This commit makes preventing the race condition. Issue 5968/1: Prevent mkdir race condition in `-dfont-ps-resdir` For `-dfont-ps-resdir`, we used mkdir after checking the directory existence. However, when multiple lilypond processes were running, as when using lilypond-book, a race condition could cause mkdir to fail. This commit uses making directory method for `-dfont-export-dir` that can prevent race condition.

Patch Set 1 #

Total comments: 10

Patch Set 2 : Improve messages #

Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -57 lines) Patch
M scm/backend-library.scm View 1 1 chunk +62 lines, -0 lines 0 comments Download
M scm/framework-ps.scm View 1 4 chunks +42 lines, -57 lines 0 comments Download

Messages

Total messages: 3
lemzwerg
LGTM, with minor nits. https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm File scm/backend-library.scm (right): https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm#newcode143 scm/backend-library.scm:143: ;; When the directory already ...
4 months, 3 weeks ago (2020-05-09 13:30:16 UTC) #1
trueroad
Improve messages
4 months, 3 weeks ago (2020-05-09 14:17:39 UTC) #2
trueroad
4 months, 3 weeks ago (2020-05-09 14:19:21 UTC) #3
Thank you for your reviewing.

https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm
File scm/backend-library.scm (right):

https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.s...
scm/backend-library.scm:143: ;; When the directory already exists, it raises
system-error.
On 2020/05/09 13:30:15, lemzwerg wrote:
> s/When/If/

Done.

https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.s...
scm/backend-library.scm:161: ;; When the file already exists, it raises
system-error.
On 2020/05/09 13:30:15, lemzwerg wrote:
> s/When/If/

Done.

https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.s...
scm/backend-library.scm:188: ;; When the file already exists, it raises
system-error.
On 2020/05/09 13:30:15, lemzwerg wrote:
> s/When/If/

Done.

https://codereview.appspot.com/561810045/diff/551840043/scm/framework-ps.scm
File scm/framework-ps.scm (right):

https://codereview.appspot.com/561810045/diff/551840043/scm/framework-ps.scm#...
scm/framework-ps.scm:284: from file `~a' index ~a...")
On 2020/05/09 13:30:15, lemzwerg wrote:
> Maybe
> 
>   for subfont ~a of file ~a")
> 
> ?

Done.

https://codereview.appspot.com/561810045/diff/551840043/scm/framework-ps.scm#...
scm/framework-ps.scm:544: (_ "Font file `~a' already exists, skipping.")
On 2020/05/09 13:30:16, lemzwerg wrote:
> s/\./.../ to be in sync with other, similar error messages.
> 
> Alternatively, you might do s/\.\.\././ in the other error messages :-)

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b