The news server INN

Ivan Shmakov | 1 Jun 2008 09:22

Picon

Re: DESTDIR support is broken (patch)

>>>>> Julien ÉLIE <julien <at> trigofacile.com> writes:

 >> One could still do relative links, they need not be in the same
 >> directory (think "..") - something along the lines of:

 >> - $(LN_S) $(MAN8)/docheckgroups.8 $D$(MAN5)/localgroups.5
 >> - $(LN_S) $(MAN8)/perl-nocem.8 $D$(MAN5)/nocem.ctl.5
 >> + $(LN_S) ../man8/docheckgroups.8 $D$(MAN5)/localgroups.5
 >> + $(LN_S) ../man8/perl-nocem.8 $D$(MAN5)/nocem.ctl.5

 > Thanks, Matija.  I have just changed that.  It is proper.

	But please note that the code above silently assumes that both
	$(MAN5) and $(MAN8) are subdirectories of a common directory and
	their ``base'' names are `man5' and `man8', respectively.  If
	for whatever reason the user building INN would set these any
	other way, the `make install' step would happily and without any
	error create a couple of dangling symlinks:

$ LC_ALL=C make MAN5=/foo/bar MAN8=/baz/qux install 2>&1 \
      | tee install.out 

	Thus, it's worthwhile to mention the assumption made at least in
	the code comments.  A better solution would be to check whether
	the assumption is actually satisfied and, if it isn't, to fall
	back creating absolute symbolic links (as was done before.)

Julien ÉLIE | 5 Jun 2008 21:14

Re: DESTDIR support is broken (patch)

Hi Ivan,

> But please note that the code above silently assumes that both
> $(MAN5) and $(MAN8) are subdirectories of a common directory and
> their ``base'' names are `man5' and `man8', respectively.

You're right.

> Thus, it's worthwhile to mention the assumption made at least in
> the code comments.  A better solution would be to check whether
> the assumption is actually satisfied and, if it isn't, to fall
> back creating absolute symbolic links (as was done before.)

Does this patch suit you?

===================================================================
--- doc/man/Makefile    (révision 7854)
+++ doc/man/Makefile    (copie de travail)
 <at>  <at>  -53,8 +53,13  <at>  <at> 
            $(CP_MAN) $$M $D$(MAN5)/$$M ; \
        done
        rm -f $D$(MAN5)/localgroups.5 $D$(MAN5)/nocem.ctl.5
-       $(LN_S) ../man8/docheckgroups.8 $D$(MAN5)/localgroups.5
-       $(LN_S) ../man8/perl-nocem.8 $D$(MAN5)/nocem.ctl.5
+       if [ -r $D$(MAN5)/../man8 ] ; then \
+           $(LN_S) ../man8/docheckgroups.8 $D$(MAN5)/localgroups.5 ; \
+           $(LN_S) ../man8/perl-nocem.8 $D$(MAN5)/nocem.ctl.5 ; \
+       else \
+           $(LN_S) $(MAN8)/docheckgroups.8 $D$(MAN5)/localgroups.5 ; \
+           $(LN_S) $(MAN8)/perl-nocem.8 $D$(MAN5)/nocem.ctl.5 ; \

(Continue reading)

Ivan Shmakov | 6 Jun 2008 03:40

Picon

Re: DESTDIR support is broken (patch)

>>>>> Julien ÉLIE <julien <at> trigofacile.com> writes:

 >> But please note that the code above silently assumes that both
 >> $(MAN5) and $(MAN8) are subdirectories of a common directory and
 >> their ``base'' names are `man5' and `man8', respectively.

 > You're right.

 >> Thus, it's worthwhile to mention the assumption made at least in the
 >> code comments.  A better solution would be to check whether the
 >> assumption is actually satisfied and, if it isn't, to fall back
 >> creating absolute symbolic links (as was done before.)

 > Does this patch suit you?

 > ===================================================================
 > --- doc/man/Makefile    (révision 7854)
 > +++ doc/man/Makefile    (copie de travail)
 >  <at>  <at>  -53,8 +53,13  <at>  <at> 
 >             $(CP_MAN) $$M $D$(MAN5)/$$M ; \
 >         done
 >         rm -f $D$(MAN5)/localgroups.5 $D$(MAN5)/nocem.ctl.5
 > -       $(LN_S) ../man8/docheckgroups.8 $D$(MAN5)/localgroups.5
 > -       $(LN_S) ../man8/perl-nocem.8 $D$(MAN5)/nocem.ctl.5
 > +       if [ -r $D$(MAN5)/../man8 ] ; then \

	I'd rather use [ $D$(MAN5)/../man8 -ef $D$(MAN8) ] here, but
	unfortunately `-ef' isn't required by POSIX.

	Also, it might be something like:

(Continue reading)

Julien ÉLIE | 6 Jun 2008 08:22

Re: DESTDIR support is broken (patch)

Hi Ivan,

> > +       if [ -r $D$(MAN5)/../man8 ] ; then \
>
> I'd rather use [ $D$(MAN5)/../man8 -ef $D$(MAN8) ] here, but
> unfortunately `-ef' isn't required by POSIX.
>
> Also, it might be something like:
>
>   if [ $(dir $(MAN5))/$(notdir $(MAN8)) = $(MAN8) ] ; then ...
>
> though I don't know whether these Make functions are portable.

Well, I can do:

    if [ "$D$(MAN5)/../man8" = "$D$(MAN8)" ] && [ -d $D$(MAN5)/../man8 ] ; then \

It will be like your suggested "-ef" (less efficient but still).

> It'll break in the following case:
>
> # ls -d /foo/man/man5
> /foo/man/man5
> # make MAN5=/foo/man/man5.my MAN8=/foo/man/man8.my install

This new test condition (= && -d) will not break in this case.

--

-- 
Julien ÉLIE

(Continue reading)

Ivan Shmakov | 6 Jun 2008 09:40

Picon

Re: DESTDIR support is broken (patch)

>>>>> Julien ÉLIE <julien <at> trigofacile.com> writes:

 >>> + if [ -r $D$(MAN5)/../man8 ] ; then \

 >> I'd rather use [ $D$(MAN5)/../man8 -ef $D$(MAN8) ] here, but
 >> unfortunately `-ef' isn't required by POSIX.

 >> Also, it might be something like:

 >> if [ $(dir $(MAN5))/$(notdir $(MAN8)) = $(MAN8) ] ; then ...

 >> though I don't know whether these Make functions are portable.

 > Well, I can do:

 > if [ "$D$(MAN5)/../man8" = "$D$(MAN8)" ] && [ -d $D$(MAN5)/../man8 ] ; then \

 > It will be like your suggested "-ef"

	But it won't!  The `-ef' operator tests whether the strings
	/name/ the same file.  On the other hand, the `=' operator tests
	whether the /strings/ are the same, and it doesn't consider any
	filesystem objects at all.  Consider, e. g.:

test /usr/share -ef /usr/share		# => TRUE
test /usr/share  =  /usr/share		# => TRUE

test /usr/share/.. -ef /usr		# => TRUE (most probably)
test /usr/share/..  =  /usr		# => FALSE

(Continue reading)

Julien ÉLIE | 6 Jun 2008 22:15

Re: DESTDIR support is broken (patch)

Hi Ivan,

>> if [ "$D$(MAN5)/../man8" = "$D$(MAN8)" ] && [ -d $D$(MAN5)/../man8 ] ; then \

Yep, I understand why it was silly to write that, sorry!
The strings are obviously not equal...

>>> I'd rather use [ $D$(MAN5)/../man8 -ef $D$(MAN8) ] here, but
>>> unfortunately `-ef' isn't required by POSIX.

Even though '-ef' is not supported on every platform, I would tend to use it.
Maybe somethink like that would be good:

 # We also create symbolic links between config files and programs.
+# If relative links can be generated, we prefer them to absolute links.
 install-man5:
        for M in $(SEC5) ; do \
            $(CP_MAN) $$M $D$(MAN5)/$$M ; \
        done
        rm -f $D$(MAN5)/localgroups.5 $D$(MAN5)/nocem.ctl.5
-       $(LN_S) ../man8/docheckgroups.8 $D$(MAN5)/localgroups.5
-       $(LN_S) ../man8/perl-nocem.8 $D$(MAN5)/nocem.ctl.5
+       $(LN_S) $(MAN8)/docheckgroups.8 $D$(MAN5)/localgroups.5
+       $(LN_S) $(MAN8)/perl-nocem.8 $D$(MAN5)/nocem.ctl.5
+       if [ $D$(MAN5)/../man8 -ef $D$(MAN8) ] ; then \
+           rm -f $D$(MAN5)/localgroups.5 $D$(MAN5)/nocem.ctl.5 ; \
+           $(LN_S) ../man8/docheckgroups.8 $D$(MAN5)/localgroups.5 ; \
+           $(LN_S) ../man8/perl-nocem.8 $D$(MAN5)/nocem.ctl.5 ; \
+       fi

(Continue reading)

Return

Return to gmane.network.inn.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane