lilypond-book: Rewrite processing of snippets

lilypond-book: Rewrite processing of snippets

The previous version relied on a stable hash() method, both for the
ordering in a set() and for the list's checksum. This probably worked
with Python 2 and up to Python 3.2, but later versions use a random
seed for invocations of hash(). This ensures different hash values for
subsequent invocations to make malicious attacks more difficult.

The new code uses hashlib.md5() and .hexdigest() instead which returns
a deterministic result across runs. It also sorts the snippets' names
which leads to more stable profiling results for 'make check'.
This change also tries to improve performance by writing snippets with
the same basename only once. Additionally it solves potential problems
if the build directory has the string '.ly' in its path.

[Dan Eble, 2020-02-01 16:54:40]

LGTM, but I'm no guru.

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

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:432: snippet_names_file = 'snippet-names-%s.ly' %
checksum
It's strange that this is named *.ly but does not contain ly code.  That's not
your problem, though.

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:481: # Sort the keys / basenames to get a stable order.
Good idea.  Is there a reason you chose not to build sorting into
process_snippets()?

[hanwenn, 2020-02-01 17:13:02]

LGTM

(didn't look very closely though)

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book.py
File scripts/lilypond-book.py (left):

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:426: return hash (' '.join([l.basename() for l in
snippets]))
alternatively, you could simply do a global search and replace of hash with
zlib.crc32

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

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:409: checksum = hashlib.md5 ()
add a comment that this is not security sensitive, so md5 is ok despite being
brokenn.

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:441: logfile = snippet_names_path[:-3]
os.path.splitext

[hahnjo, 2020-02-02 09:52:23]

Address comments & revert temporary workaround in master

[hahnjo, 2020-02-02 09:52:37]

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

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:409: checksum = hashlib.md5 ()
On 2020/02/01 17:13:02, hanwenn wrote:
> add a comment that this is not security sensitive, so md5 is ok despite being
> brokenn.

Done.

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:432: snippet_names_file = 'snippet-names-%s.ly' %
checksum
On 2020/02/01 16:54:40, Dan Eble wrote:
> It's strange that this is named *.ly but does not contain ly code.  That's not
> your problem, though.

True. It's now relatively simple to change, what would you prefer?

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:441: logfile = snippet_names_path[:-3]
On 2020/02/01 17:13:02, hanwenn wrote:
> os.path.splitext

Done.

https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
scripts/lilypond-book.py:481: # Sort the keys / basenames to get a stable order.
On 2020/02/01 16:54:40, Dan Eble wrote:
> Good idea.  Is there a reason you chose not to build sorting into
> process_snippets()?

Only a weak one: Keep it close to the code that uniques the list. Eventually we
could use a dict that also sorts its keys.

[Dan Eble, 2020-02-02 13:21:58]

On 2020/02/02 09:52:37, hahnjo wrote:
>
https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
> scripts/lilypond-book.py:432: snippet_names_file = 'snippet-names-%s.ly' %
> checksum
> On 2020/02/01 16:54:40, Dan Eble wrote:
> > It's strange that this is named *.ly but does not contain ly code.  That's
not
> > your problem, though.
> 
> True. It's now relatively simple to change, what would you prefer?

I'd probably have chosen txt.

New topic: Do you recommend any manual steps to clean up the workspace before
running tests with these changes, or should it work seamlessly?

[hahnjo, 2020-02-02 13:26:04]

On 2020/02/02 13:21:58, Dan Eble wrote:
> On 2020/02/02 09:52:37, hahnjo wrote:
> >
>
https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
> > scripts/lilypond-book.py:432: snippet_names_file = 'snippet-names-%s.ly' %
> > checksum
> > On 2020/02/01 16:54:40, Dan Eble wrote:
> > > It's strange that this is named *.ly but does not contain ly code.  That's
> not
> > > your problem, though.
> > 
> > True. It's now relatively simple to change, what would you prefer?
> 
> I'd probably have chosen txt.

Ok, will do.

> New topic: Do you recommend any manual steps to clean up the workspace before
> running tests with these changes, or should it work seamlessly?

It should complete without issues, but you'll again see many profiling
differences because of the changed order. After this change has landed, you
should recreate a test-baseline.

[hahnjo, 2020-02-02 15:24:33]

snippet-names-*.ly -> .txt

[hahnjo, 2020-02-02 15:28:25]

Revert change to .txt

[hahnjo, 2020-02-02 15:33:05]

On 2020/02/02 13:26:04, hahnjo wrote:
> On 2020/02/02 13:21:58, Dan Eble wrote:
> > On 2020/02/02 09:52:37, hahnjo wrote:
> > >
> >
>
https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book...
> > > scripts/lilypond-book.py:432: snippet_names_file = 'snippet-names-%s.ly' %
> > > checksum
> > > On 2020/02/01 16:54:40, Dan Eble wrote:
> > > > It's strange that this is named *.ly but does not contain ly code. 
That's
> > not
> > > > your problem, though.
> > > 
> > > True. It's now relatively simple to change, what would you prefer?
> > 
> > I'd probably have chosen txt.
> 
> Ok, will do.

Meh: I did the change and it works, but subprocess_system in python/lilylib.py
will still output the .ly name. As this file doesn't exist anymore, it will just
make investigations in case of an error more difficult. I propose to table that
change for now, ok?

[Dan Eble, 2020-02-02 18:00:20]

On 2020/02/02 15:33:05, hahnjo wrote:
> > > I'd probably have chosen txt.
> > 
> > Ok, will do.
> 
> Meh: I did the change and it works, but subprocess_system in python/lilylib.py
> will still output the .ly name. As this file doesn't exist anymore, it will
just
> make investigations in case of an error more difficult. I propose to table
that
> change for now, ok?

Not a problem.  Kudos for trying it.