Tidies binary relocation

I found this code while trying to work out how to get lilypond to run from the command line with the windows PATH statement.  There are pointless if statements, and the use of a now undocumented environment variable.  This patch tidies that up.

[PhilEHolmes, 2012-08-18 14:08:43]

Please review

[Graham Percival, 2012-08-18 14:19:23]

I haven't looked at the new logic in detail, but exactly what was
LILYPOND_RELOCATE_PREFIX doing, and why do we no longer need it?  If that was
the environment variable that allowed me to build the documentation on osx
without compiling lilypond, then I would be sad to see it go.

[mail_philholmes.net, 2012-08-18 14:40:26]

----- Original Message ----- 
From: <graham@percival-music.ca>
To: <PhilEHolmes@googlemail.com>; <dak@gnu.org>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Saturday, August 18, 2012 3:19 PM
Subject: Re: Tidies binary relocation (issue 6463066)


>I haven't looked at the new logic in detail, but exactly what was
> LILYPOND_RELOCATE_PREFIX doing, and why do we no longer need it?  If
> that was the environment variable that allowed me to build the
> documentation on osx without compiling lilypond, then I would be sad to
> see it go.
>
>
> http://codereview.appspot.com/6463066/


TBH, I have no idea.  It relies on an environment variable that has no 
mention in our docs anywhere, and the only reference I can find to it at all 
is an email message from 2006, where using it didn't fix the problem anyway.

If anyone can say what it's used for, we could document it.  But I don't 
think we should rely on reading the C++ code to find all the environment 
variables we could use...

The main reason I started to edit this is because it had the construct:

if (x)
{
  if (x)
  {
  }
}

which doesn't seem too good.  Whilst trying to work out what that was doing, 
I concluded the relocate bit couldn't do anything either, since no-one would 
know about it.

--
Phil Holmes 

[Graham Percival, 2012-08-18 18:30:00]

On Sat, Aug 18, 2012 at 03:40:19PM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: <graham@percival-music.ca>
> To: <PhilEHolmes@googlemail.com>; <dak@gnu.org>
> Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
> 
> >I haven't looked at the new logic in detail, but exactly what was
> >LILYPOND_RELOCATE_PREFIX doing, and why do we no longer need it?  If
> 
> TBH, I have no idea.  It relies on an environment variable that has
> no mention in our docs anywhere, and the only reference I can find
> to it at all is an email message from 2006, where using it didn't
> fix the problem anyway.

That doesn't necessarily anything.  It could be used in GUB for
building, or lilypad-osx or lilypad-windows for compiling.  I must
admit that I've grepped for it in both git repositories and
couldn't find it, though.  It might even be involved in the
lilypond jail settings (i.e. for LSR).  (and note that unless you
have personally set up a working LSR, do not assume that the docs
on lilypond jail actually work!)

> If anyone can say what it's used for, we could document it.  But I
> don't think we should rely on reading the C++ code to find all the
> environment variables we could use...

OTOH, I don't think that we should delete bits of code unless we
know why they were there in the first place.  There's just so many
dark corners of lilypond that no active developers are familiar
with.


I'm not saying that I'm completely opposed to having this in git
master.  I'd feel better if you made some binaries and asked users
to test them first.  But even without that, as long as you'll be
around in Sep to revert this patch if necessary, I think it could
still go forward.

- Graham

[email_philholmes.net, 2012-08-19 09:27:45]

----- Original Message ----- 
From: "Graham Percival" <graham@percival-music.ca>
To: "Phil Holmes" <mail@philholmes.net>
Cc: <PhilEHolmes@googlemail.com>; <dak@gnu.org>; <lilypond-devel@gnu.org>; 
<reply@codereview-hr.appspotmail.com>
Sent: Saturday, August 18, 2012 7:29 PM
Subject: Re: Tidies binary relocation (issue 6463066)


> On Sat, Aug 18, 2012 at 03:40:19PM +0100, Phil Holmes wrote:
>> ----- Original Message ----- From: <graham@percival-music.ca>
>> To: <PhilEHolmes@googlemail.com>; <dak@gnu.org>
>> Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
>>
>> >I haven't looked at the new logic in detail, but exactly what was
>> >LILYPOND_RELOCATE_PREFIX doing, and why do we no longer need it?  If
>>
>> TBH, I have no idea.  It relies on an environment variable that has
>> no mention in our docs anywhere, and the only reference I can find
>> to it at all is an email message from 2006, where using it didn't
>> fix the problem anyway.
>
> That doesn't necessarily anything.  It could be used in GUB for
> building, or lilypad-osx or lilypad-windows for compiling.  I must
> admit that I've grepped for it in both git repositories and
> couldn't find it, though.  It might even be involved in the
> lilypond jail settings (i.e. for LSR).  (and note that unless you
> have personally set up a working LSR, do not assume that the docs
> on lilypond jail actually work!)

I did the same.  It's not just that it doean't appear anywhere in our build 
system - it doesn't appear anywhere on t'internet, either.

>> If anyone can say what it's used for, we could document it.  But I
>> don't think we should rely on reading the C++ code to find all the
>> environment variables we could use...
>
> OTOH, I don't think that we should delete bits of code unless we
> know why they were there in the first place.  There's just so many
> dark corners of lilypond that no active developers are familiar
> with.

I think we should do some sweeping up in cobwebby corners, if we can.  In 
particular, when we can't find any reason for the code to be there in the 
first place.  I've not worked out yet how to get git to show which commit 
put that code in.  If someone could either do that, or explain how to, I 
think it could help.

> I'm not saying that I'm completely opposed to having this in git
> master.  I'd feel better if you made some binaries and asked users
> to test them first.  But even without that, as long as you'll be
> around in Sep to revert this patch if necessary, I think it could
> still go forward.


I've made binaries and tested them under Ubuntu.  The code is not windows 
specific, it's OS independent.  It relies on someone setting the environment 
value LILYPOND_RELOCATE_PREFIX which could work on any OS.  So I think the 
only way to get enough people to test would be to put it into a release and 
see if anyone complains.  I would revert in that case.

--
Phil Holmes 

[dak, 2012-08-19 09:32:50]

"Phil Holmes" <email@philholmes.net> writes:

> I think we should do some sweeping up in cobwebby corners, if we can.
> In particular, when we can't find any reason for the code to be there
> in the first place.  I've not worked out yet how to get git to show
> which commit put that code in.  If someone could either do that, or
> explain how to, I think it could help.

git grep LILYPOND_RELOCATE_PREFIX
Documentation/misc/ChangeLog-2.10:      * lily/main.cc (setup_paths): fix:
LILYPOND_RELOCATE_PREFIX.
Documentation/misc/ChangeLog-2.10:      * lily/main.cc (setup_paths): read
LILYPOND_RELOCATE_PREFIX
lily/relocate.cc:      if (getenv ("LILYPOND_RELOCATE_PREFIX"))
lily/relocate.cc:          prefix_directory = getenv
("LILYPOND_RELOCATE_PREFIX");

git gui blame lily/relocate.cc
/RELOCATE_PREFIX

Click on line.  There are some cases:

commit 38e0fd809d01de319d40224b5d2279888304b2d3
Author:	Jan Nieuwenhuizen <janneke@gnu.org>  Fri Feb  2 13:06:30 2007
Committer:	Jan Nieuwenhuizen <janneke@gnu.org>  Fri Feb  2 13:06:30 2007

Finish rename of LILYPONDPREFIX to LILYPOND_DATADIR.  Remove confusion
between prefix (/usr) and lilypond_datadir (/usr/share/lilypond/x.y.x),
fixes running from compile prefix and build tree with `current' link.


commit 38e0fd809d01de319d40224b5d2279888304b2d3
Author:	Jan Nieuwenhuizen <janneke@gnu.org>  Fri Feb  2 13:06:30 2007
Committer:	Jan Nieuwenhuizen <janneke@gnu.org>  Fri Feb  2 13:06:30 2007

Finish rename of LILYPONDPREFIX to LILYPOND_DATADIR.  Remove confusion
between prefix (/usr) and lilypond_datadir (/usr/share/lilypond/x.y.x),
fixes running from compile prefix and build tree with `current' link.



-- 
David Kastrup

[pkx166h, 2012-08-19 09:33:53]

On 19 Aug 2012, at 10:27, "Phil Holmes" <email@philholmes.net> wrote:
--
--

> ----- Original Message ----- From: "Graham Percival"
<graham@percival-music.ca>
> To: "Phil Holmes" <mail@philholmes.net>
> Cc: <PhilEHolmes@googlemail.com>; <dak@gnu.org>; <lilypond-devel@gnu.org>;
<reply@codereview-hr.appspotmail.com>
> Sent: Saturday, August 18, 2012 7:29 PM
> Subject: Re: Tidies binary relocation (issue 6463066)
> 
> 
>> On Sat, Aug 18, 2012 at 03:40:19PM +0100, Phil Holmes wrote:
>>> ----- Original Message ----- From: <graham@percival-music.ca>
>>> To: <PhilEHolmes@googlemail.com>; <dak@gnu.org>
>>> Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
>>> 
>>> >I haven't looked at the new logic in detail, but exactly what was
>>> >LILYPOND_RELOCATE_PREFIX doing, and why do we no longer need it?  If
>>> 
>>> TBH, I have no idea.  It relies on an environment variable that has
>>> no mention in our docs anywhere, and the only reference I can find
>>> to it at all is an email message from 2006, where using it didn't
>>> fix the problem anyway.
>> 
>> That doesn't necessarily anything.  It could be used in GUB for
>> building, or lilypad-osx or lilypad-windows for compiling.  I must
>> admit that I've grepped for it in both git repositories and
>> couldn't find it, though.  It might even be involved in the
>> lilypond jail settings (i.e. for LSR).  (and note that unless you
>> have personally set up a working LSR, do not assume that the docs
>> on lilypond jail actually work!)
> 
> I did the same.  It's not just that it doean't appear anywhere in our build
system - it doesn't appear anywhere on t'internet, either.

Seriously? Peter Kay memes?

It's the future!

James

[Graham Percival, 2012-08-19 13:15:52]

On Sun, Aug 19, 2012 at 10:27:37AM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: "Graham Percival"
> <graham@percival-music.ca>
> To: "Phil Holmes" <mail@philholmes.net>
> >OTOH, I don't think that we should delete bits of code unless we
> >know why they were there in the first place.  There's just so many
> >dark corners of lilypond that no active developers are familiar
> >with.
> 
> I think we should do some sweeping up in cobwebby corners, if we
> can.  In particular, when we can't find any reason for the code to
> be there in the first place.  I've not worked out yet how to get git
> to show which commit put that code in.  If someone could either do
> that, or explain how to, I think it could help.

gitk lily/relocate.cc lily/include/relocate.hh

you can list as many (or as few) files as you want to get only the
history specific to those file(s).  Adding an entire directory
will give you everything for all files in that directory (and
subdirectories).

> >I'm not saying that I'm completely opposed to having this in git
> >master.  I'd feel better if you made some binaries and asked users
> >to test them first.  But even without that, as long as you'll be
> >around in Sep to revert this patch if necessary, I think it could
> >still go forward.
> 
> I've made binaries and tested them under Ubuntu.  The code is not
> windows specific, it's OS independent.  It relies on someone setting
> the environment value LILYPOND_RELOCATE_PREFIX which could work on
> any OS.

Yes, but that environment variable could be set up by the OSX .app
or the windows installer.  I agree that in this case it seems
unlikely since we haven't found that string in the lilypad or gub
repositories, though.  But I wanted to note that environment
variables don't require the user to set them up specifically.

>  So I think the only way to get enough people to test would
> be to put it into a release and see if anyone complains.  I would
> revert in that case.

There *is* another way: build binaries, upload to your server,
email -user asking for volunteers to test them.  I used to do this
quite a bit when I was working on GUB back in Fall 2009.  Yes,
it's more work, but OTOH it meant that I wasn't treating git
master as an testing ground.  The lilypond developer community is
getting pickier about only putting patches into master if there's
evidence that they do no harm -- that's the whole point of the
reviews and Patchy.  When it comes to build system stuff,
unfortunately there's no way to test it on a single platform.

That said, don't take this as a vote against the patch.  I'm ok
with it going through.

- Graham