bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: new module: relpath


From: Pádraig Brady
Subject: Re: new module: relpath
Date: Sat, 16 Jun 2012 17:15:02 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20110816 Thunderbird/6.0

On 06/16/2012 04:25 PM, Bruno Haible wrote:
> Hi Akim,
> 
> Akim Demaille wrote:
>> In Bison, I consider computing relative paths between files
>> (one concrete use is when the user creates the parser implementation
>> file in --output=sub1/sub2/parser.c and the header file in
>> --defines=sub3/sub4/parser.h, in which case parser.c should include
>> "../../sub3/sub4/parser.h").
> 
> I agree that
>   1. this is general-purpose functionality that has its place in gnulib,
>   2. when you start to copy code from coreutils to bison, it's time to
>      move it to gnulib, to avoid duplicate maintenance.

Me too. Thanks Akim for looking at this!

> About the module name 'relpath': The GNU standards, section "GNU Manuals",
> say
>    Please do not use the term "pathname" that is used in Unix
>    documentation; use "file name" (two words) instead.  We use the term
>    "path" only for search paths, which are lists of directory names.
> 
> Strictly speaking this is only about manuals, not about code. But the
> more we use the term "file name" also in code, the less we are tempted
> to misuse the term "path" in documentation. Gnulib already has modules
>   concat-filename
>   filename
>   filenamecat
> It would be good to choose a module name in this spirit.

I suppose relname would be most appropriate given that argument,
though I think in this case it's slightly better to keep the relpath name
so as to align with similar functions elsewhere like
os.path.relpath() and relpath(1)?

>> * lib/relpath.c: New.
>> * lib/relpath.h: New.
>> * modules/relpath: New.
> 
> When you contribute a gnulib module, it's also welcome to write a unit
> test: 2 files tests/test-relpath.c and modules/relpath-tests.
> 
>> +License:
>> +GPLv3+
> 
> Please write 'GPL' here. It is semantically equivalent to 'GPLv3+', but
> gnulib-tool does not understand 'GPLv3+'.
> 
>> +++ b/modules/relpath
> 
>> +Depends-on:
>> +canonicalize
>> +dirname
>> +error
>> +pathmax
>> +stdbool
>> +xalloc
>> +...
>> +License:
>> +GPLv3+
> 
> This module may produce error messages and be under GPL. Then the module
> is not usable in shared libraries. If you want a module that is more widely
> usable, consider an interface that returns error codes  or error strings,
> remove the dependencies to 'error' and 'xalloc', and change the license
> to 'LGPL'.
> 
>> +/* Output the relative representation if possible.
>> +   If BUF is non NULL, write to that buffer rather than to stdout.  */
> 
> This comment is incomplete. It should tell what is the meaning of each
> argument (CAN_FNAME, CAN_RELDIR, BUF if == NULL, LEN) and what is the
> meaning of the return value ('bool' here: does true mean success or does
> it mean failure?).
> 
> Also, I find it odd to have a function that produces either a string or
> some output (on a fixed stream, that is not even passed as argument).
> For a gnulib module it would be more appropriate to have only the string
> case. Users who need to output it to stdout can do that after invoking
> the function.

Well there are 2 uses in coreutils at present.
ln wants the string returned, but the realpath command
just wants relpath() to output to stdout,
to avoid the malloc and data copy overhead.
If you wanted to make it a bit more general, I guess you
could pass in the stream rather than defaulting to stdout.
coreutils can adjust to changes in interface here anyway.

>> +/* Return FROM represented as relative to the dir of TARGET.
>> +   The result is malloced.  */
>> +
>> +char *
>> +convert_abs_rel (const char *from, const char *target);
> 
> It is a bit strange here that if I want to compute a relative filename,
> relative to a given directory, I will have to add "/." to this TARGET
> directory. How about either
q>   - adding a second function that takes a directory as second argument
>     (as opposed to a file in this directory), or
>   - change the specification of convert_abs_rel in this way, outright?
>     (Then many callers will have to perform a dirname() themselves.)

Yes this function needs a bit of work as it's quite specific
to ln's needs at present. It also includes a PATH_MAX allocation
which isn't appropriate for gnulib.

>> +#include <config.h>
>> +
>> +#include <stdbool.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <stdio.h>
>> +#include <errno.h>
>> +
>> +#include "gettext.h"
>> +#define _(msgid) gettext (msgid)
>> +
>> +#include "canonicalize.h"
>> +#include "dirname.h"
>> +#include "error.h"
>> +#include "relpath.h"
>> +#include "xalloc.h"
> 
> By convention, the specification header ("relpath.h" in this case) should
> be included right after <config.h>. This helps verifying that the include
> file is self-contained, i.e. does not need a prerequisite of <sys/types.h>
> or similar.

cheers,
Pádraig.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]