Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File::Spec::Unix->catfile("./a") ne File::Spec::Unix->catfile(".", "a") #12232

Closed
p5pRT opened this issue Jun 29, 2012 · 10 comments
Closed

File::Spec::Unix->catfile("./a") ne File::Spec::Unix->catfile(".", "a") #12232

p5pRT opened this issue Jun 29, 2012 · 10 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Jun 29, 2012

Migrated from rt.perl.org#113898 (status was 'rejected')

Searchable as RT113898$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 29, 2012

From perl@profvince.com

Created by perl@profvince.com

  $ perl -MFile​::Spec​::Unix -le 'print File​::Spec​::Unix->catfile("./a")'
  a
  $ perl -MFile​::Spec​::Unix -le 'print File​::Spec​::Unix->catfile(".",
"a")'
  ./a

I don't think those two should be different, but they are. Which form
should be returned by catfile()? The second one has the advantage of
being more shell-friendly : when 'a' is not in your PATH, you can do
"system './a'" but not "system 'a'". On the other hand the first form
comes straight from canonpath() :
  $ perl -MFile​::Spec​::Unix -le'print File​::Spec​::Unix->canonpath("./a")'
  a

and I suspect that changing this to './a' could cause a lot of breakage.

Perl Info

Flags:
     category=library
     severity=low
     module=File::Spec

Site configuration information for perl 5.16.0:

Configured by Gentoo at Thu Jun 14 18:45:58 CEST 2012.

Summary of my perl5 (revision 5 version 16 subversion 0) configuration:
      Platform:
     osname=linux, osvers=3.4.0-fuuka.profvince.com, archname=x86_64-linux
     uname='linux fuuka 3.4.0-fuuka.profvince.com #1 smp sat jun 2 
15:42:13 cest 2012 x86_64 intel(r) core(tm)2 duo cpu e6750 @ 2.66ghz 
genuineintel gnulinux '
     config_args='-des -Duseshrplib -Darchname=x86_64-linux 
-Dcc=x86_64-pc-linux-gnu-gcc -Doptimize=-march=core2 -O3 
-fomit-frame-pointer -ftree-vectorize -ftree-vectorizer-verbose=1 -pipe 
-DNDEBUG -Dldflags=-Wl,-O1 -Wl,--as-needed -Dprefix=/usr 
-Dinstallprefix=/usr -Dsiteprefix=/usr/local -Dvendorprefix=/usr 
-Dscriptdir=/usr/bin -Dprivlib=/usr/lib64/perl5/5.16.0 
-Darchlib=/usr/lib64/perl5/5.16.0/x86_64-linux 
-Dsitelib=/usr/local/lib64/perl5/5.16.0 
-Dsitearch=/usr/local/lib64/perl5/5.16.0/x86_64-linux 
-Dvendorlib=/usr/lib64/perl5/vendor_perl/5.16.0 
-Dvendorarch=/usr/lib64/perl5/vendor_perl/5.16.0/x86_64-linux 
-Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 
-Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 
-Dvendorman1dir=/usr/share/man/man1 -Dvendorman3dir=/usr/share/man/man3 
-Dman1ext=1 -Dman3ext=3pm -Dlibperl=libperl.so.5.16.0 
-Dlocincpth=/usr/include  -Dglibpth=/lib64 /usr/lib64  -Duselargefiles 
-Dd_semctl_semun -Dcf_by=Gentoo -Dmyhostname=localhost 
-Dperladmin=root@localhost -Dinstallusrbinperl=n -Ud_csh -Uusenm 
-Di_ndbm -Di_gdbm -Di_db -DDEBUGGING=none -Dlibpth=/usr/local/lib64 
/lib64 /usr/lib64 -Dnoextensions=ODBM_File'
     hint=recommended, useposix=true, d_sigaction=define
     useithreads=undef, usemultiplicity=undef
     useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
     use64bitint=define, use64bitall=define, uselongdouble=undef
     usemymalloc=n, bincompat5005=undef
   Compiler:
     cc='x86_64-pc-linux-gnu-gcc', ccflags ='-fno-strict-aliasing -pipe 
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
     optimize='-march=core2 -O3 -fomit-frame-pointer -ftree-vectorize 
-ftree-vectorizer-verbose=1 -pipe -DNDEBUG',
     cppflags='-fno-strict-aliasing -pipe'
     ccversion='', gccversion='4.6.3', gccosandvers=''
     intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
     d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
     ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', 
lseeksize=8
     alignbytes=8, prototype=define
   Linker and Libraries:
     ld='x86_64-pc-linux-gnu-gcc', ldflags ='-Wl,-O1 -Wl,--as-needed'
     libpth=/usr/local/lib64 /lib64 /usr/lib64
     libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
     perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
     libc=/lib/libc-2.15.so, so=so, useshrplib=true, 
libperl=libperl.so.5.16.0
     gnulibc_version='2.15'
   Dynamic Linking:
     dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
     cccdlflags='-fPIC', lddlflags='-shared -march=core2 -O3 
-fomit-frame-pointer -ftree-vectorize -ftree-vectorizer-verbose=1 -pipe 
-DNDEBUG -Wl,-O1 -Wl,--as-needed'

Locally applied patches:


@INC for perl 5.16.0:
     /etc/perl
     /usr/local/lib64/perl5/5.16.0/x86_64-linux
     /usr/local/lib64/perl5/5.16.0
     /usr/lib64/perl5/vendor_perl/5.16.0/x86_64-linux
     /usr/lib64/perl5/vendor_perl/5.16.0
     /usr/lib64/perl5/5.16.0/x86_64-linux
     /usr/lib64/perl5/5.16.0
     .


Environment for perl 5.16.0:
     HOME=/home/vince
     LANG=fr_FR.UTF-8
     LANGUAGE (unset)
     LC_ALL=fr_FR.UTF-8
     LD_LIBRARY_PATH (unset)
     LOGDIR (unset)
 
PATH=/home/vince/bin:/home/vince/perl/builds/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/4.6.3:/opt/intel/cce/10.1.018/bin:/usr/games/bin
     PERL_BADLANG (unset)
     SHELL=/bin/bash

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 30, 2012

From @jkeenan

On Fri Jun 29 06​:02​:21 2012, perl@​profvince.com wrote​:

This is a bug report for perl from perl@​profvince.com,
generated with the help of perlbug 1.39 running under perl 5.16.0.

 $ perl \-MFile​::Spec​::Unix \-le 'print File​::Spec​::Unix\-

catfile("./a")'
a
$ perl -MFile​::Spec​::Unix -le 'print File​::Spec​::Unix-
catfile(".",
"a")'
./a

I don't think those two should be different, but they are. Which form
should be returned by catfile()?

I don't think this is a bug. I think your second instance is a correct
instance of the behavior set forth in the File​::Spec documentation.

"File​::Spec​::Unix​::catfile​:

Concatenate one or more directory names and a filename to form a
complete path ending with a filename"

The documentation -- and this is true of File​::Spec itself, as well --
specifies that catfile() ought to take a non-zero list of directories
and a filename. Your second instance above does this.

Your first instance provides a relative path to a filename in the first
argument. The fact that this usage supplies something that may be
useful under some circumstances is ... interesting, but it's not a use
of catfile() which conforms to that function's documentation.

If others differ, then we should discuss. But I don't think this is a
real bug.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 30, 2012

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 1, 2012

From @ikegami

On Sat, Jun 30, 2012 at 2​:32 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

I don't think this is a bug. I think your second instance is a correct
instance of the behavior set forth in the File​::Spec documentation.

I disagree about the lack of bug, even though the behaviour is technically
within the bounds of documented behaviour.

catfile, as standard for File​::Spec, normally returns a canonized path.
Only when the leading part is "." does it not return a partially canonised
path.

perl -MFile​::Spec​::Unix -E"say File​::Spec​::Unix->catfile('a')"
a

perl -MFile​::Spec​::Unix -E"say File​::Spec​::Unix->catfile('.', 'a')"
./a

perl -MFile​::Spec​::Unix -E"say File​::Spec​::Unix->catfile('.', '.', 'a')"
./a

perl -MFile​::Spec​::Unix -E"say File​::Spec​::Unix->catfile('.', '.', '.',
'a')"
./a

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 1, 2012

From perl@profvince.com

I don't think this is a bug. I think your second instance is a correct
instance of the behavior set forth in the File​::Spec documentation.

"File​::Spec​::Unix​::catfile​:

Concatenate one or more directory names and a filename to form a
complete path ending with a filename"

The documentation -- and this is true of File​::Spec itself, as well --
specifies that catfile() ought to take a non-zero list of directories
and a filename. Your second instance above does this.

I hadn't even realized that the documentation required at least one
directory entry, given the case of "no directory" is handled explicitly
both in catfile()'s implementation and tests. However, I'm pretty sure
many people write catfile(@​dirs, $file) without checking that @​dirs is
not empty. So, practically speaking, catfile() has to handle that case.

I can see several solutions :

- 1°) catfile() should always return canonical paths stripped from their
leading ./ when they are relative. It means that catfile() would
actually be reimplemented as canonpath(join '/', @​_). This is
consistent, but sometimes you need to prepend curdir() to a path in a
portable way, like when you want pass an executable to system() (as in
my original report).

- 2°) catfile() should always return canonical paths starting with ./
when they are relative. Theoretically, this is the best solution but I
suspect it would break a lot of tests on CPAN that compare what
catfile() returns against an hard-coded path.

- 3°) catfile() should just concatenate its arguments, without
canonifying leadings "./". This is consistent (as far as catfile() is
concerned), has limited breakage potential, and still allow to prepend a
dot when needed, so it is my preferred way of fixing the issue.

To summarize, here's what catfile() returns for each of these strategies
(0 being the current state of affairs) :

  +-----+-----+-----+-----+
  | 0 | 1 | 2 | 3 |
+-------------------+-----+-----+-----+-----+
| catfile('a') | a | a | ./a | a |
+-------------------+-----+-----+-----+-----+
| catfile('./a') | a | a | ./a | ./a |
+-------------------+-----+-----+-----+-----+
| catfile('.', 'a') | ./a | a | ./a | ./a |
+-------------------+-----+-----+-----+-----+

I have implemented solution 3°) in the topic branch
vincent/normalize_unix_catfile in the repository, and I will smoke it
against the CPAN when the machine is available.

Vincent

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 24, 2012

From @rjbs

* Vincent Pit <perl@​profvince.com> [2012-07-01T09​:38​:19]

- 3°) catfile() should just concatenate its arguments, without
canonifying leadings "./". This is consistent (as far as catfile()
is concerned), has limited breakage potential, and still allow to
prepend a dot when needed, so it is my preferred way of fixing the
issue.

I agree with your preference.

--
rjbs

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 24, 2013

From @jkeenan

On Sun Jul 01 06​:38​:57 2012, perl@​profvince.com wrote​:

I don't think this is a bug. I think your second instance is a correct
instance of the behavior set forth in the File​::Spec documentation.

"File​::Spec​::Unix​::catfile​:

Concatenate one or more directory names and a filename to form a
complete path ending with a filename"

The documentation -- and this is true of File​::Spec itself, as well --
specifies that catfile() ought to take a non-zero list of directories
and a filename. Your second instance above does this.

I hadn't even realized that the documentation required at least one
directory entry, given the case of "no directory" is handled explicitly
both in catfile()'s implementation and tests. However, I'm pretty sure
many people write catfile(@​dirs, $file) without checking that @​dirs is
not empty. So, practically speaking, catfile() has to handle that case.

I can see several solutions :

- 1�) catfile() should always return canonical paths stripped from their
leading ./ when they are relative. It means that catfile() would
actually be reimplemented as canonpath(join '/', @​_). This is
consistent, but sometimes you need to prepend curdir() to a path in a
portable way, like when you want pass an executable to system() (as in
my original report).

- 2�) catfile() should always return canonical paths starting with ./
when they are relative. Theoretically, this is the best solution but I
suspect it would break a lot of tests on CPAN that compare what
catfile() returns against an hard-coded path.

- 3�) catfile() should just concatenate its arguments, without
canonifying leadings "./". This is consistent (as far as catfile() is
concerned), has limited breakage potential, and still allow to prepend a
dot when needed, so it is my preferred way of fixing the issue.

To summarize, here's what catfile() returns for each of these strategies
(0 being the current state of affairs) :

                 \+\-\-\-\-\-\+\-\-\-\-\-\+\-\-\-\-\-\+\-\-\-\-\-\+
                 |  0  |  1  |  2  |  3  |

+-------------------+-----+-----+-----+-----+
| catfile('a') | a | a | ./a | a |
+-------------------+-----+-----+-----+-----+
| catfile('./a') | a | a | ./a | ./a |
+-------------------+-----+-----+-----+-----+
| catfile('.', 'a') | ./a | a | ./a | ./a |
+-------------------+-----+-----+-----+-----+

I have implemented solution 3�) in the topic branch
vincent/normalize_unix_catfile in the repository, and I will smoke it
against the CPAN when the machine is available.

Vincent

Vincent,

Did you get smoke results? Should we continue to explore this solution?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 24, 2013

From perl@profvince.com

Vincent,

Did you get smoke results?

I must confess that it has been a long time since I've looked into this.
I seem to recall that an incomplete cpan smoke yielded some code that
would be broken by the change, but I don't remember if there was a lot
of it.

Should we continue to explore this solution?

Probably, yes. The issue is still there.

Thank you very much.
Jim Keenan

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=113898

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 14, 2017

From zefram@fysh.org

All uses of catfile that conform to the API produce consistent results.
While I'm sympathetic to the argument that ->catfile ought to be producing
a canonical result on general principle, the documentation doesn't
claim that it'll do that, so it's certainly not a bug that it doesn't.
Given the ambiguous nature of the documentation and the existence of
CPAN code that depends on the present form of output, I'm inclined to
not change it. This ticket should be closed.

-zefram

@p5pRT p5pRT closed this as completed Dec 14, 2017
@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 14, 2017

@iabyn - Status changed from 'open' to 'rejected'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant