Re: [PATCH 1/3] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'

Johannes Sixt
Zitat von Steffen Prohaska <[hidden email]>:

>
> On Jul 27, 2008, at 11:24 PM, Shawn O. Pearce wrote:
>
> > Steffen Prohaska <[hidden email]> wrote:
> >> The new execdir has is two levels below the root directory, while
> >> the old execdir 'bin' was only one level below.  This commit
> >> adapts the discovery of oguilib that uses relative paths
> >> accordingly.
> > ...
> >> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> >> index 940677c..baccd57 100755
> >> --- a/git-gui/git-gui.sh
> >> +++ b/git-gui/git-gui.sh
> >> @@ -52,7 +52,9 @@ catch {rename send {}} ; # What an evil concept...
> >> set oguilib {@@GITGUI_LIBDIR@@}
> >> set oguirel {@@GITGUI_RELATIVE@@}
> >> if {$oguirel eq {1}} {
> >> - set oguilib [file dirname [file dirname [file normalize $argv0]]]
> >> + set oguilib [file dirname \
> >> +             [file dirname \
> >> +              [file dirname [file normalize $argv0]]]]
> >> set oguilib [file join $oguilib share git-gui lib]
> >
> > Hmmph.  This actually comes up incorrectly on my system.  The issue
> > appears to be `git --exec-path` gives me $prefix/libexec/git-core,
> > and git-gui installs its library into $prefix/libexec/share, which
> > is wrong.  It should have gone to $prefix/share.
>
> I am not seeing this problem because I am installing using the
> toplevel makefile, which sets and exports sharedir to $prefix/share.
>
>
> > I wonder if this is better.  Your other two patches seem fine.
> >
> > --8<--
> > [PATCH] git-gui: Correct installation of library to be $prefix/share
> >
> > We always wanted the library for git-gui to install into the
> > $prefix/share directory, not $prefix/libexec/share.  All of
> > the files in our library are platform independent and may
> > be reused across systems, like any other content stored in
> > the share directory.
> >
> > Our computation of where our library should install to was broken
> > when git itself started installing to $prefix/libexec/git-core,
> > which was one level down from where we expected it to be.
> >
> > Signed-off-by: Shawn O. Pearce <[hidden email]>
> > ---
> > Makefile |    3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index b19fb2d..f72ab6c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -32,6 +32,9 @@ endif
> > ifndef gitexecdir
> > gitexecdir := $(shell git --exec-path)
> > endif
> > +ifeq (git-core,$(notdir $(gitexecdir)))
> > + gitexecdir := $(patsubst %/,%,$(dir $(gitexecdir)))
> > +endif
>
> But gitexecdir has the correct value, no?  gitexecdir is used
> at several places in the makefile.  It seems wrong to strip
> 'git-core' from gitexecdir.  But I must admit that I do not
> understand all the details of git-gui's Makefile.  So maybe
> you know better.
>
> Isn't only the computation of sharedir based on gitexecdir wrong?
>
> > ifndef sharedir
> > sharedir := $(dir $(gitexecdir))share
>
>
> and could be replaced with this (instead of your patch):
>
>   ifndef sharedir
> +ifeq (git-core,$(notdir $(gitexecdir)))
> +       sharedir := $(dir $(patsubst %/,%,$(dir $(gitexecdir))))share
> +else
>          sharedir := $(dir $(gitexecdir))share
>   endif
> +endif

This is not good enough in my environment. I run git-gui effectivly with

   wish $prefix/libexec/git-core/git-gui

(and I have $PATH set up to contain $bindir, but not $gitexecdir), and this
needs the original hunk with the three [file dirname ... ], because $argv0
points to $prefix/libexec/git-core/git-gui.

I thought I understood what's going on, but I don't anymore.

Mybe the relative discovery of oguilib must be conditional on the "git-core"
part as well, just like you discover sharedir?

-- Hannes


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'

Steffen Prohaska

On Aug 3, 2008, at 11:35 AM, Johannes Sixt wrote:

> Zitat von Steffen Prohaska <[hidden email]>:
>>
>> On Jul 27, 2008, at 11:24 PM, Shawn O. Pearce wrote:
>>
>>> Steffen Prohaska <[hidden email]> wrote:
>>>> The new execdir has is two levels below the root directory, while
>>>> the old execdir 'bin' was only one level below.  This commit
>>>> adapts the discovery of oguilib that uses relative paths
>>>> accordingly.
>>> ...
>>>> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
>>>> index 940677c..baccd57 100755
>>>> --- a/git-gui/git-gui.sh
>>>> +++ b/git-gui/git-gui.sh
>>>> @@ -52,7 +52,9 @@ catch {rename send {}} ; # What an evil  
>>>> concept...
>>>> set oguilib {@@GITGUI_LIBDIR@@}
>>>> set oguirel {@@GITGUI_RELATIVE@@}
>>>> if {$oguirel eq {1}} {
>>>> - set oguilib [file dirname [file dirname [file normalize $argv0]]]
>>>> + set oguilib [file dirname \
>>>> +             [file dirname \
>>>> +              [file dirname [file normalize $argv0]]]]
>>>> set oguilib [file join $oguilib share git-gui lib]
>>>
>>> Hmmph.  This actually comes up incorrectly on my system.  The issue
>>> appears to be `git --exec-path` gives me $prefix/libexec/git-core,
>>> and git-gui installs its library into $prefix/libexec/share, which
>>> is wrong.  It should have gone to $prefix/share.
>>
>> I am not seeing this problem because I am installing using the
>> toplevel makefile, which sets and exports sharedir to $prefix/share.
>>
>>
>>> I wonder if this is better.  Your other two patches seem fine.
>>>
>>> --8<--
>>> [PATCH] git-gui: Correct installation of library to be $prefix/share
>>>
>>> We always wanted the library for git-gui to install into the
>>> $prefix/share directory, not $prefix/libexec/share.  All of
>>> the files in our library are platform independent and may
>>> be reused across systems, like any other content stored in
>>> the share directory.
>>>
>>> Our computation of where our library should install to was broken
>>> when git itself started installing to $prefix/libexec/git-core,
>>> which was one level down from where we expected it to be.
>>>
>>> Signed-off-by: Shawn O. Pearce <[hidden email]>
>>> ---
>>> Makefile |    3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index b19fb2d..f72ab6c 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -32,6 +32,9 @@ endif
>>> ifndef gitexecdir
>>> gitexecdir := $(shell git --exec-path)
>>> endif
>>> +ifeq (git-core,$(notdir $(gitexecdir)))
>>> + gitexecdir := $(patsubst %/,%,$(dir $(gitexecdir)))
>>> +endif
>>
>> But gitexecdir has the correct value, no?  gitexecdir is used
>> at several places in the makefile.  It seems wrong to strip
>> 'git-core' from gitexecdir.  But I must admit that I do not
>> understand all the details of git-gui's Makefile.  So maybe
>> you know better.
>>
>> Isn't only the computation of sharedir based on gitexecdir wrong?
>>
>>> ifndef sharedir
>>> sharedir := $(dir $(gitexecdir))share
>>
>>
>> and could be replaced with this (instead of your patch):
>>
>>  ifndef sharedir
>> +ifeq (git-core,$(notdir $(gitexecdir)))
>> +       sharedir := $(dir $(patsubst %/,%,$(dir $(gitexecdir))))share
>> +else
>>         sharedir := $(dir $(gitexecdir))share
>>  endif
>> +endif
>
> This is not good enough in my environment.

This only fixes the installation directory.


> I run git-gui effectivly with
>
>   wish $prefix/libexec/git-core/git-gui
>
> (and I have $PATH set up to contain $bindir, but not $gitexecdir),  
> and this
> needs the original hunk with the three [file dirname ... ], because  
> $argv0
> points to $prefix/libexec/git-core/git-gui.

The original hunk fixes the discovery of oguilib, i.e.
from $prefix/libexec/git-core/git-gui to $prefix/share/git-gui/lib

I didn't recognize that the the 'three [file dinames ...]' have
not been applied because I had this change already in 4msysgit
(and still have).  Apologies for not checking this more carefully.


> I thought I understood what's going on, but I don't anymore.
>
> Mybe the relative discovery of oguilib must be conditional on the  
> "git-core"
> part as well, just like you discover sharedir?

Hmm... you are right.  If we want to maintain compatibility
with *both* directory layouts, obviously all computations
that depend on the layout need to be conditional on it.

        Steffen

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'

Johannes Sixt
The new execdir has is two levels below the root directory, while
the old execdir 'bin' was only one level below.  This commit
adapts the discovery of oguilib that uses relative paths
accordingly. We determine whether we have the extra level in the same
way in which the Makefile defines sharedir, i.e. whether the last
directory part is 'git-core'.

Inspired-by: Steffen Prohaska <[hidden email]>
Signed-off-by: Johannes Sixt <[hidden email]>
---

Zitat von Steffen Prohaska <[hidden email]>:

>
> On Aug 3, 2008, at 11:35 AM, Johannes Sixt wrote:
> > I run git-gui effectivly with
> >
> >   wish $prefix/libexec/git-core/git-gui
> >
> > (and I have $PATH set up to contain $bindir, but not $gitexecdir),
> > and this
> > needs the original hunk with the three [file dirname ... ], because
> > $argv0
> > points to $prefix/libexec/git-core/git-gui.
>
> The original hunk fixes the discovery of oguilib, i.e.
> from $prefix/libexec/git-core/git-gui to $prefix/share/git-gui/lib
>
> I didn't recognize that the the 'three [file dinames ...]' have
> not been applied because I had this change already in 4msysgit
> (and still have).  Apologies for not checking this more carefully.
>
>
> > I thought I understood what's going on, but I don't anymore.
> >
> > Mybe the relative discovery of oguilib must be conditional on the
> > "git-core"
> > part as well, just like you discover sharedir?
>
> Hmm... you are right.  If we want to maintain compatibility
> with *both* directory layouts, obviously all computations
> that depend on the layout need to be conditional on it.

Here is a patch that does just this.

-- Hannes

 git-gui/git-gui.sh |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index fd3875a..fa08d49 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -52,7 +52,11 @@ catch {rename send {}} ; # What an evil concept...
 set oguilib {@@GITGUI_LIBDIR@@}
 set oguirel {@@GITGUI_RELATIVE@@}
 if {$oguirel eq {1}} {
- set oguilib [file dirname [file dirname [file normalize $argv0]]]
+ set oguilib [file dirname [file normalize $argv0]]
+ if {[lindex [file split $oguilib] end] eq {git-core}} {
+ set oguilib [file dirname $oguilib]
+ }
+ set oguilib [file dirname $oguilib]
  set oguilib [file join $oguilib share git-gui lib]
  set oguimsg [file join $oguilib msgs]
 } elseif {[string match @@* $oguirel]} {
--
1.6.0.rc1.958.gce1ed


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'

Shawn Pearce
Johannes Sixt <[hidden email]> wrote:

> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index fd3875a..fa08d49 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -52,7 +52,11 @@ catch {rename send {}} ; # What an evil concept...
>  set oguilib {@@GITGUI_LIBDIR@@}
>  set oguirel {@@GITGUI_RELATIVE@@}
>  if {$oguirel eq {1}} {
> - set oguilib [file dirname [file dirname [file normalize $argv0]]]
> + set oguilib [file dirname [file normalize $argv0]]
> + if {[lindex [file split $oguilib] end] eq {git-core}} {
> + set oguilib [file dirname $oguilib]

Isn't that what [file tail] does?

        if {[file tail $oguilib] eq {git-core}} {

But otherwise this looks good to me.  I'll apply it later tonight,
unless you agree that file tail is better here.

--
Shawn.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'

Johannes Sixt
The new execdir has is two levels below the root directory, while
the old execdir 'bin' was only one level below.  This commit
adapts the discovery of oguilib that uses relative paths
accordingly. We determine whether we have the extra level in the same
way in which the Makefile defines sharedir, i.e. whether the last
directory part is 'git-core'.

Inspired-by: Steffen Prohaska <[hidden email]>
Signed-off-by: Johannes Sixt <[hidden email]>
---

Zitat von "Shawn O. Pearce" <[hidden email]>:
> Johannes Sixt <[hidden email]> wrote:
> > + if {[lindex [file split $oguilib] end] eq {git-core}} {
>
> Isn't that what [file tail] does?

Yes, it is. I missed it in the myriad of file's options. Here's the updated
version of the patch.

-- Hannes

 git-gui/git-gui.sh |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index fd3875a..60f376d 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -52,7 +52,11 @@ catch {rename send {}} ; # What an evil concept...
 set oguilib {@@GITGUI_LIBDIR@@}
 set oguirel {@@GITGUI_RELATIVE@@}
 if {$oguirel eq {1}} {
- set oguilib [file dirname [file dirname [file normalize $argv0]]]
+ set oguilib [file dirname [file normalize $argv0]]
+ if {[file tail $oguilib] eq {git-core}} {
+ set oguilib [file dirname $oguilib]
+ }
+ set oguilib [file dirname $oguilib]
  set oguilib [file join $oguilib share git-gui lib]
  set oguimsg [file join $oguilib msgs]
 } elseif {[string match @@* $oguirel]} {
--
1.6.0.rc1.958.gb3893


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html