Re: New Feature Submission for GNU Make
Howard, Thank you for your feedback. I think an example would best justify the improvements that trimpath and relpath would bring to GNU Make. In fact, I will give the actual example that motivated me to develop trimpath the relpath in the first place. We can generalize from that starting point. First a little background... My build system is developed using GNU Make and it separates the build system repository, from the project's source tree/repository that makes use of it. This allows multiple teams, developing different projects, in different repositories to incorporate the features of this build system, separate from the team's project repository. As a result, I need some pathing variables to traverse back and forth between the repositories, which result in long relative paths. I have basically applied the FTSE to the build system. Here is an sample compiler invocation: g++ -g -O0 -fshort-wchar -DGTEST_USE_OWN_TR1_TUPLE=1 -I. -I../../../_make_interface/../../../_make_system/source/_make_interface/../common/headers - I../../../_make_interface/../../../xseries/source/_make_interface/../common/headers - I../../../_make_interface/../../../xseries/source/_make_interface/../common/libs/algorithms - I../../../_make_interface/../../../xseries/source/_make_interface/../common/libs/asserting - I../../../_make_interface/../../../xseries/source/_make_interface/../common/libs/mathfuncs - I../../../_make_interface/../../../xseries/source/_make_interface/../common/libs/protocols - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/controllers - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/discrete - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/filters - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/flowcassette - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/linear - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/lookuptables - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/nonlinear - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlblocks/patterngenerators - I../../../_make_interface/../../../xseries/source/_make_interface/../vent/libs/controlframework -c nonlinear/AbsDifferenceBlock.cpp -o _debug/tdd/obj_controlblocks/AbsDifferenceBlock.cpp.o It became obvious to me that these paths were unnecessarily long, and I desired an API to shorten them (I could not shorten the generation of these paths without giving up certain features of the build system). Applying $(relpath ... ) to these search paths results in the following compiler invocation: g++ -g -O0 -fshort-wchar -DGTEST_USE_OWN_TR1_TUPLE=1 -I. -I../../../../../_make_system/source/common/headers -I../../../common/headers -I../../../common/libs/algorithms - ../../../common/libs/asserting -I../../../common/libs/mathfuncs -I../../../common/libs/protocols -I../../../vent/libs/controlblocks -I../../../vent/libs/controlblocks/controllers -I../../../vent/libs/controlblocks/discrete -I../../../vent/libs/controlblocks/filters -I../../../vent/libs/controlblocks/flowcassette - I../../../vent/libs/controlblocks/linear -I../../../vent/libs/controlblocks/lookuptables -I../../../vent/libs/controlblocks/nonlinear -I../../../vent/libs/controlblocks/patterngenerators -I../../../vent/libs/controlframework -c nonlinear/AbsDifferenceBlock.cpp -o _debug/tdd/obj_controlblocks/AbsDifferenceBlock.cpp.o This was much easier to read, likely faster to execute, and better overall. Now to answer your two points: 1) Yes, if my directory structure is built with "weirdly symlinked build trees", than I should not use a function to remove seemingly unnecessary pathing elements. In general, I would advise against "weirdly symlinked build trees". :) This function still has great usability otherwise. 2) Certain compilers have an input buffer size limitation, and this function can put the compiler invocation under that limit. I agree that is not a good solution in general, and if I continued to add more paths, I would eventually end up over that limit. From a practical perspective however, relpath put me far below that limit, when I was far above that limit, and was practically very useful. Regards, Ben Robinson, Ph.D. On Mon, May 30, 2011 at 12:27 PM, Howard Chu wrote: > Ben Robinson wrote: > >> Eli, >> >> Thank you for your constructive criticism regarding my submission to GNU >> Make. >> I perceive the critiques to fall into three categories (documentation, >> justification and code improvements) which I will respond to in order. >> >> *Documentation*: You are correct, these functions remove only "redundant" >> or >>
Re: New Feature Submission for GNU Make
Ben Robinson wrote: Eli, Thank you for your constructive criticism regarding my submission to GNU Make. I perceive the critiques to fall into three categories (documentation, justification and code improvements) which I will respond to in order. *Documentation*: You are correct, these functions remove only "redundant" or "unnecessary" . and .. components. The suggested documentation should instead read: $(trimpath names|...|) For each file name innames,returns a name that does not contain any|unnecessary.|or|..|components, nor any repeated path separators (|/|). Canonical absolute names remain canonical absolute, and relative names remain relative. $(relpath names|...|) For each file name innames,returns the relative path to the file. This relative path does not contain anyunnecessary |.|or|..|components, nor any repeated path separators (|/|). *Justification (trimpath)*: trimpath can be used to shorten the input strings to compilers/linkers, as well as improve readability to the user. Certain compilers have a maximum number of characters which can be passed into a single invocation of their compiler. In my project, I had a dozen or so -include which contained many unnecessary . and .. components, which caused the compiler to overflow the input buffer. While it is unfortunate compilers exist with unreasonably small input buffers, trimpath allowed me to only pass in the minimum number of characters required to successfully compile. Pretty weak. If a few more include paths were added to the project it would still break, regardless of your patch. Also, the readability of paths printed to the console is greatly improved by trimpath. I was regularly dealing with paths of the following structure: -I../../../_make_interface/../../../_make_system/source/_make_implementation/../3rdparty/libs/gtest/include which would be reduced by trimpath to: -I../../../../../_make_system/source/3rdparty/libs/gtest/include At first glance this sounds like a good thing, but it seems to miss the possibility of weirdly symlinked build trees, where "foo/bar/.." != "foo". -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/ ___ Bug-make mailing list Bug-make@gnu.org https://lists.gnu.org/mailman/listinfo/bug-make
Re: New Feature Submission for GNU Make
Eli, Thank you for your constructive criticism regarding my submission to GNU Make. I perceive the critiques to fall into three categories (documentation, justification and code improvements) which I will respond to in order. *Documentation*: You are correct, these functions remove only "redundant" or "unnecessary" . and .. components. The suggested documentation should instead read: $(trimpath names...) For each file name in names, returns a name that does not contain any unnecessary . or .. components, nor any repeated path separators (/). Canonical absolute names remain canonical absolute, and relative names remain relative. $(relpath names...) For each file name in names, returns the relative path to the file. This relative path does not contain any unnecessary . or .. components, nor any repeated path separators (/). *Justification (trimpath)*: trimpath can be used to shorten the input strings to compilers/linkers, as well as improve readability to the user. Certain compilers have a maximum number of characters which can be passed into a single invocation of their compiler. In my project, I had a dozen or so -include which contained many unnecessary . and .. components, which caused the compiler to overflow the input buffer. While it is unfortunate compilers exist with unreasonably small input buffers, trimpath allowed me to only pass in the minimum number of characters required to successfully compile. Also, the readability of paths printed to the console is greatly improved by trimpath. I was regularly dealing with paths of the following structure: -I../../../_make_interface/../../../_make_system/source/_make_implementation/../3rdparty/libs/gtest/include which would be reduced by trimpath to: -I../../../../../_make_system/source/3rdparty/libs/gtest/include abspath could have made these paths longer depending on the depth into the filesystem that the project was being built. *Justification (relpath)*: Similiar to trimpath, relpath can be used to shorten the input strings to compilers/linkers, as well as improve readability to the user. For example, in my build system, libraries are "published" relative to a known directory, and paths to these libraries are built up for each executable that needs them. Consider an executable foo which links to library bar. If foo and bar exist in a directory name directoryname, then the relative path to library bar would be" -L../bar However, the build system generates: -L../../_make_interface/../directoryname/bar which relpath reduces back to: -L../bar As a note, trimpath would only reduce the path in this case to: -L../../directoryname/bar abspath and realpath could potentially create: -L/home/benjamin.robinson/workspace/xseries/source/directoryname/bar *Code Improvements*: I agree with the coding suggestions you have made. I will refactor my submission to incorporate your suggestions, provided Paul agrees the submission is justified. A small note: my implementation is based on abspath from version 3.81 of function.c which did not use IS_ABSOLUTE, ROOT_LEN, IS_PATHSEP, etc... I will refactor my submission to use them, along with your other suggestions, provided it passes the justification gate. And I will further clarify my intent in the code comments where you indicated. Thank you, Ben Robinson, Ph.D. On Mon, May 30, 2011 at 12:23 AM, Eli Zaretskii wrote: > > Date: Sun, 29 May 2011 21:45:38 -0700 > > From: Ben Robinson > > > > I am submitting a patch which adds two new functions to GNU Make. > > Thanks. Please wait for Paul's word about inclusion of these in > Make. What's below are a few comments based on a first impression > from the code. > > > $(trimpath names...) > > For each file name in names, returns a name that does not contain any . > or > > .. components, nor any repeated path separators (/). > > The part about "." and ".." is inaccurate: the function removes any > redundant "." and "..", but it's not true that the value will never > include these, as your tests clearly show: > > > ifneq ($(trimpath ../foo/),../foo) > > $(error ) > > endif > > > $(relpath names...) > > For each file name in names, returns the relative path to the file. > > This doesn't say relative to what. From reading the code I understand > that it's relative to the starting directory (which could be different > from the current directory when a given recipe runs). Why not use > $(CURDIR) instead? > > > This > > relative path does not contain any . or .. components, nor any repeated > path > > separators (/). > > Again, this is inaccurate: periods will still appear when they are > needed. > > > These functions contain capabilities which are significantly different > from > > the existing abspath and realpath, in that they do not convert what could > be > > an extremely short relative path (e.g. ".") into a long absolute path. > > relpath is in fact the inverse of absp
Re: New Feature Submission for GNU Make
> Date: Sun, 29 May 2011 21:45:38 -0700
> From: Ben Robinson
>
> I am submitting a patch which adds two new functions to GNU Make.
Thanks. Please wait for Paul's word about inclusion of these in
Make. What's below are a few comments based on a first impression
from the code.
> $(trimpath names...)
> For each file name in names, returns a name that does not contain any . or
> .. components, nor any repeated path separators (/).
The part about "." and ".." is inaccurate: the function removes any
redundant "." and "..", but it's not true that the value will never
include these, as your tests clearly show:
> ifneq ($(trimpath ../foo/),../foo)
> $(error )
> endif
> $(relpath names...)
> For each file name in names, returns the relative path to the file.
This doesn't say relative to what. From reading the code I understand
that it's relative to the starting directory (which could be different
from the current directory when a given recipe runs). Why not use
$(CURDIR) instead?
> This
> relative path does not contain any . or .. components, nor any repeated path
> separators (/).
Again, this is inaccurate: periods will still appear when they are
needed.
> These functions contain capabilities which are significantly different from
> the existing abspath and realpath, in that they do not convert what could be
> an extremely short relative path (e.g. ".") into a long absolute path.
> relpath is in fact the inverse of abspath, and adding it would make the set
> of path conversion functions more complete. In addition, trimpath can be
> applied to paths both absolute and relative, and eliminate needless
> characters which can improve readability and performance.
Any use case where adding these functions would be needed?
Completeness is not generally enough to add features.
A few comments on the code, mostly related to Windows portability:
> > if (name[0] == '/') {
> > ++fixed;
> > abspath = 1;
> > }
This is not the GNU style of using braces (here and everywhere else in
the patch).
> > /* A Windows style absolute path */
> > if (name[1] == ':' && name[2] == '/') {
> > fixed += 3;
> > abspath = 1;
> > }
Please use the existing IS_ABSOLUTE and ROOT_LEN macros.
Also, I'm not sure the Windows parts should be compiled on Posix
platforms, since it is possible (although unlikely) to have a file or
directory there named "C:".
> > /* Take only the first path-separator. */
> > if (*start == '/') {
> > ++start;
> > *dest++ = '/';
> > }
Please don't assume the directory separator is always '/', it could be
'\\' on DOS/Windows. Please use IS_PATHSEP instead of literal
slashes.
> > /* Skip sequence of multiple path-separators. */
> > while (*start == '/') {
> > ++start;
> > }
This will defeat UNC "//foo/bar" or "foo\\bar" file names on
Windows.
> > /* Strip the trailing separator if any. */
> > if (dest > apath && dest[-1] == '/') {
> > /* Unless name is an absolute path resulting in only '/' */
> > if (!(name[0] == '/' && dest == apath + 1)) {
> > --dest;
This does not consider the DOS/Windows case where an absolute file
name does not begin with a slash.
> > /* If the resulting path is empty, return a '.' */
> > if (dest == apath) {
> > *dest++ = '.';
> > }
Same here.
> > /* A unix style absolute path */
> > if (name[0] == '/') {
> > abspath = 1;
> > if (starting_directory[1] == ':' && starting_directory[2] == '/') {
> > /* A Windows system should not get passed a unix style absolute path
> > */
> > return NULL;
What happens if starting_directory is a "//foo/bar" UNC?
> > /* A Windows style absolute path */
> > if (name[1] == ':' && name[2] == '/') {
Again, please use IS_ABSOLUTE.
> > if (name[0] != starting_directory[0]) {
> > /* Cannot convert a path on a different drive letter to relative */
> > return NULL;
??? Why not? simply return the original name without any changes, it's
better than failing.
> > if (strlen(tname) != 1 && tname[0] != '/') {
> > strcat(tname, "/");
> > }
This again works only on Unix.
> > /* Skip common characters in both paths */
> > while (*srcname == *srcdir && *srcname != '\0' && *srcdir != '\0') {
> > ++srcname;
> > ++srcdir;
> > }
> > /* Now rewind to last common / */
> > while (*srcname != '/' && *srcdir != '/' && srcdir != starting_directory)
> > {
> > --srcname;
> > --srcdir;
> > }
What will happen here if the argument of relpath is exactly identical
to starting_directory?
Also, the test `srcdir != starting_directory' will not DTRT on
Windows, because the first character is not a slash.
> > /* If the resulting path is empty, return a '.' */
> > if (dest == apath) {
> > *dest++ = '.';
> > }
Not TRT on Windows.
Thanks again for working on this.
___
Bug-
