Fix Issue 2146 "Illegal entry in bfrange block in ToUnicode CMap"

Fix Issue 2146 "Illegal entry in bfrange block in ToUnicode CMap"

[Graham Percival, 2012-07-15 00:21:16]

wow, awesome work!

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itexi
File Documentation/common-macros.itexi (right):

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itex...
Documentation/common-macros.itexi:16: % code stolen from Heiko Oberdiek's
`ifpdf' package
sorry to nitpick, but could you use the word "copied" instead?  there's no
benefit to waving a red flag to anybody doing casual greps and looking for
trouble.

http://codereview.appspot.com/6399046/diff/1/configure.in
File configure.in (right):

http://codereview.appspot.com/6399046/diff/1/configure.in#newcode66
configure.in:66: _NCSB_SOURCE_FILES_=""
what's the convention for leading _ in names?  I havent' encountered this
before.

http://codereview.appspot.com/6399046/diff/1/scripts/build/lys-to-tely.py
File scripts/build/lys-to-tely.py (right):

http://codereview.appspot.com/6399046/diff/1/scripts/build/lys-to-tely.py#new...
scripts/build/lys-to-tely.py:69: %%%% code stolen from Heiko Oberdiek's `ifpdf'
package
ditto

[John Mandereau, 2012-07-15 00:37:15]

Hi Graham,

Thanks for the quick review, I'll upload another patch when I get another review
or at the earliest this week-end.

Please also note my comments on the bug tracker w.r.t. distribution of
lilypond.map and documentation.

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itexi
File Documentation/common-macros.itexi (right):

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itex...
Documentation/common-macros.itexi:16: % code stolen from Heiko Oberdiek's
`ifpdf' package
On 2012/07/15 00:21:16, Graham Percival wrote:
> sorry to nitpick, but could you use the word "copied" instead?  there's no
> benefit to waving a red flag to anybody doing casual greps and looking for
> trouble.

OK, I had not thought of that.

http://codereview.appspot.com/6399046/diff/1/configure.in
File configure.in (right):

http://codereview.appspot.com/6399046/diff/1/configure.in#newcode66
configure.in:66: _NCSB_SOURCE_FILES_=""
On 2012/07/15 00:21:16, Graham Percival wrote:
> what's the convention for leading _ in names?  I havent' encountered this
> before.

It was just my lack of imagination for a variable that should go through
filtering (namely checking for "Cyrillic") before being AC_SUBSTed. If using an
underscore is really not appropriate, then I'll invent some variable name.

[Graham Percival, 2012-07-15 01:41:15]

On Sun, Jul 15, 2012 at 12:37:15AM +0000, john.mandereau@gmail.com wrote:
> configure.in:66: _NCSB_SOURCE_FILES_=""
> On 2012/07/15 00:21:16, Graham Percival wrote:
> >what's the convention for leading _ in names?  I havent' encountered
> this
> >before.
> 
> It was just my lack of imagination for a variable that should go through
> filtering (namely checking for "Cyrillic") before being AC_SUBSTed. If
> using an underscore is really not appropriate, then I'll invent some
> variable name.

I wouldn't say that it was "really not appropriate", but it looked
to me like a special GNUMakefile or stepmake convention.  Maybe
call it NCSB_SOURCE_FILES_UNFILTERED or something like that?

- Graham

[dak, 2012-07-15 02:54:13]

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itexi
File Documentation/common-macros.itexi (right):

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itex...
Documentation/common-macros.itexi:16: % code stolen from Heiko Oberdiek's
`ifpdf' package
On 2012/07/15 00:37:15, John Mandereau wrote:
> On 2012/07/15 00:21:16, Graham Percival wrote:
> > sorry to nitpick, but could you use the word "copied" instead?  there's no
> > benefit to waving a red flag to anybody doing casual greps and looking for
> > trouble.
> 
> OK, I had not thought of that.

Unless I am mistaken, ifpdf would have been released under the LPPL, and we
release under the incompatible GPL.

Just write
\ifpdf \pdfmapfile... \fi

since Texinfo defined \ifpdf for us.

[dak, 2012-07-15 03:10:04]

http://codereview.appspot.com/6399046/diff/1/configure.in
File configure.in (right):

http://codereview.appspot.com/6399046/diff/1/configure.in#newcode66
configure.in:66: _NCSB_SOURCE_FILES_=""
On 2012/07/15 00:37:15, John Mandereau wrote:
> On 2012/07/15 00:21:16, Graham Percival wrote:
> > what's the convention for leading _ in names?  I havent' encountered this
> > before.
> 
> It was just my lack of imagination for a variable that should go through
> filtering (namely checking for "Cyrillic") before being AC_SUBSTed. If using
an
> underscore is really not appropriate, then I'll invent some variable name.

Why does the underscore help here?  The answer should likely be a code comment
as well.

[lemzwerg, 2012-07-18 16:44:50]

This looks great!  I don't have time right now to test it, but errors will be
quite easily be caught :-)  Thanks for fixing this.

[John Mandereau, 2012-07-18 23:47:13]

On 2012/07/18 16:44:50, lemzwerg wrote:
> This looks great!  I don't have time right now to test it, but errors will be
> quite easily be caught :-)  Thanks for fixing this.

I've uploaded a new patch that matches Git index and that integrated suggestions
from reviews, but comments like "Running dvips may produce some warnings about
fonts; these are harmless and may be ignored. " in
http://lilypond.org/doc/v2.15/Documentation/usage/invoking-lilypond_002dbook....

make me think properly documenting usage of lilypond.map in Usage would be much
more work, so let's fix the issue first.

[Graham Percival, 2012-07-20 08:48:32]

LGTM

[dak, 2012-07-20 09:04:28]

On 2012/07/20 08:48:32, Graham Percival wrote:
> LGTM

I am somewhat concerned (meaning that I have no clue whatsoever, but a queasy
feeling) about the implications for running/installing LilyPond in various
directories.  Will this do the right thing for testing with and without separate
build directory, for GUB, and the typical fake-root build schemes employed by
RPM and apt?

It looks like it nails down some absolute paths in the configuration.

[John Mandereau, 2012-07-20 09:34:09]

On 2012/07/20 09:04:28, dak wrote:
> On 2012/07/20 08:48:32, Graham Percival wrote:
> > LGTM
> 
> I am somewhat concerned (meaning that I have no clue whatsoever, but a queasy
> feeling) about the implications for running/installing LilyPond in various
> directories.  Will this do the right thing for testing with and without
separate
> build directory, for GUB, and the typical fake-root build schemes employed by
> RPM and apt?
> 
> It looks like it nails down some absolute paths in the configuration.

Your concerns don't apply to a use of lilypond.map limited to building LilyPond
documentation, which I think I handle correctly with this patch w.r.t. seperate
build directory or not and GUB (as long as GUB makes available NCSB fonts to
configure script).

Your concerns would apply if we installed and distributed lilypond.map, but I
don't include this in this patch, because I don't want to do it without document
it, and feel to bad with the current state of lilypond-book documentation w.r.t.
fonts handling with tex documents (it roughly says that warning about fonts may
be ignored) to edit it without a boatload of testing with
lilypond-book/tex/texinfo samples, which may require from me extra initial work
understanding how fonts selection and inclusion works.

I propose to fix the issue w.r.t. LilyPond doc build, closing issue 2146, and
not fuss with distributing lilypond.map until I can investigate this with help
of experts like you and Werner.

[dak, 2012-07-20 09:45:10]

On 2012/07/20 09:34:09, John Mandereau wrote:

> I propose to fix the issue w.r.t. LilyPond doc build, closing issue 2146, and
> not fuss with distributing lilypond.map until I can investigate this with help
> of experts like you and Werner.

Ah ok, I was not aware that this was a doc-build only patch, so it seems
reasonably safe to go ahead here.

[PhilEHolmes, 2012-07-20 15:48:23]

On 2012/07/20 09:45:10, dak wrote:
> On 2012/07/20 09:34:09, John Mandereau wrote:
> 
> > I propose to fix the issue w.r.t. LilyPond doc build, closing issue 2146,
and
> > not fuss with distributing lilypond.map until I can investigate this with
help
> > of experts like you and Werner.
> 
> Ah ok, I was not aware that this was a doc-build only patch, so it seems
> reasonably safe to go ahead here.

Do we need to update GUB with the lilypond-map?

[Graham Percival, 2012-07-20 16:32:50]

On Fri, Jul 20, 2012 at 03:48:25PM +0000, PhilEHolmes@googlemail.com wrote:
> On 2012/07/20 09:45:10, dak wrote:
> >Ah ok, I was not aware that this was a doc-build only patch, so it
> seems
> >reasonably safe to go ahead here.
> 
> Do we need to update GUB with the lilypond-map?

Short answer is "I don't know".

Medium answer is "I don't know, but please test it".

Long answer is "I don't know, but please test it, and hopefully
the abc.com guy gets back from summer vacation (presumably?) so
that you can build the real version yourself"?


My own release stuff requires a reboot and not using that computer
for a few hours (during which time I can't use that computer for
any real work due to the old OS), so I'd rather not have this
uncertainty hanging around when I try to build the next release
candidate.

- Graham

[mail_philholmes.net, 2012-07-20 16:41:13]

----- Original Message ----- 
From: "Graham Percival" <graham@percival-music.ca>
To: <john.mandereau@gmail.com>; <dak@gnu.org>; <lemzwerg@googlemail.com>; 
<PhilEHolmes@googlemail.com>; <lilypond-devel@gnu.org>; 
<reply@codereview-hr.appspotmail.com>
Sent: Friday, July 20, 2012 5:32 PM
Subject: Re: Fix Issue 2146 "Illegal entry in bfrange block in 
ToUnicodeCMap" (issue 6399046)


> On Fri, Jul 20, 2012 at 03:48:25PM +0000, PhilEHolmes@googlemail.com 
> wrote:
>> On 2012/07/20 09:45:10, dak wrote:
>> >Ah ok, I was not aware that this was a doc-build only patch, so it
>> seems
>> >reasonably safe to go ahead here.
>>
>> Do we need to update GUB with the lilypond-map?
>
> Short answer is "I don't know".
>
> Medium answer is "I don't know, but please test it".


If someone can let me know how I should update my GUB installation, I'll run 
a build on it as soon as I've tried the update.  I think this would work 
best with this patch on a branch on git so I can build GUB from that branch.

--
Phil Holmes 

[Graham Percival, 2012-07-20 17:53:07]

On Fri, Jul 20, 2012 at 05:41:05PM +0100, Phil Holmes wrote:
> If someone can let me know how I should update my GUB installation,
> I'll run a build on it as soon as I've tried the update.  I think
> this would work best with this patch on a branch on git so I can
> build GUB from that branch.

If the patch is in git origin (any branch), then you can build
from that branch by substituting the branch name for
release/unstable or stable/2.14 in the minor release checklist.

There's also some way of building from a local git repo, but I
don't know how to do that, and we might as well do it from the
main repo.

- Graham

[mail_philholmes.net, 2012-07-21 09:33:16]

----- Original Message ----- 
From: "Graham Percival" <graham@percival-music.ca>
To: "Phil Holmes" <mail@philholmes.net>
Cc: <john.mandereau@gmail.com>; <dak@gnu.org>; <lemzwerg@googlemail.com>; 
<lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Friday, July 20, 2012 6:53 PM
Subject: Re: Fix Issue 2146 "Illegal entry in bfrange block in 
ToUnicodeCMap" (issue 6399046)


> On Fri, Jul 20, 2012 at 05:41:05PM +0100, Phil Holmes wrote:
>> If someone can let me know how I should update my GUB installation,
>> I'll run a build on it as soon as I've tried the update.  I think
>> this would work best with this patch on a branch on git so I can
>> build GUB from that branch.
>
> If the patch is in git origin (any branch), then you can build
> from that branch by substituting the branch name for
> release/unstable or stable/2.14 in the minor release checklist.
>
> There's also some way of building from a local git repo, but I
> don't know how to do that, and we might as well do it from the
> main repo.
>
> - Graham
>

I'm working on pushing the patch to dev/philh, but every time I do something 
like this it's a new adventure in git.  Once I've got that going, I'll build 
GUB from that branch.  FWIW, inspection of the code makes it appear to me 
that this will update GUB as well.

--
Phil Holmes