test broken on cygwin #3

Closed
mokko opened this Issue Nov 30, 2012 · 13 comments

Projects

None yet

3 participants

@mokko
mokko commented Nov 30, 2012

I tried Alien::Base right now, but File::chdir doesn't test well on my cygwin... I start looking into it now.

looks like mkdir makes a directory, but one which has no /n in it, it returns success anyways.
local $CWD = $Test_Dir fails because dir with /n in name doesn't exist.

not sure where the error is and how to solve it.

PERL_DL_NONLAZY=1 /usr/bin/perl.exe "-MExtUtils::Command::MM" "-e" "test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/00-compile.t .... ok
t/array.t ......... ok
t/chdir.t ......... ok
t/delete-array.t .. ok
t/nested.t ........ ok
t/newline.t ....... Cannot chdir back to /home/maurice/projects/File-chdir-0.1007/t/testdir3492: No such file or directory at /home/maurice/projects/File-chdir-0.1007/blib/lib/File/chdir.pm line 21.
t/newline.t ....... Dubious, test returned 2 (wstat 512, 0x200)
No subtests run
t/var.t ........... ok
@dagolden

Thanks for looking into it. File::chdir is a PITA. You could also suggest that Alien::Base switch to File::pushd, which works fine on Cygwin.

@mokko
mokko commented Dec 1, 2012

will do. Thx for fast feedback.

@mokko mokko referenced this issue in Perl5-Alien/Alien-Base Dec 1, 2012
Closed

File::chdir and File::pushd on cygwin #19

@jberger
Member
jberger commented Dec 1, 2012

After several conversations along these lines, I still find myself liking File::chdir more than File::pushd, the latter being far less powerful. For example, I can keep moving up a tree easiiy:

local $CWD = $self->alien_temp_dir;
# ... download and extract a tarball here
push @CWD, $ae->extract_path
# ... build etc

or I can change multiple directories by

local $CWD;
push @CWD, qw/t system_installed/;

rather than

File::pushd->new( File::Spec->catdir( qw/ t system_installed / ) );

All that said, how much work does File::chdir need? I'm willing to help (though I'm stretched pretty thin as it is right now).

@jberger
Member
jberger commented Dec 1, 2012

I can't help but notice that the error being thrown by File::chdir (checking that chdir succeeds) is not checked by File::pushd. As the same function (CORE::chdir) is doing the chdir-ing, I cannot see how one succeeds and the other fails.

@jberger
Member
jberger commented Dec 1, 2012

On further digging, not only does File::pushd not check the status of chdir, but the test suite does not test for directories with newlines anyway. Thus I'm rather certain that the reason File::pushd tests succeed where File::chdir does not is that the tests are not broad enough for the latter, not that that the former is inferior.

Another nit, File::chdir's t/newline.t creates a nested directory in a distinctly unsatisfying way. I would think that one would want do

local $CWD = 't';
my $Test_Dir = "testdir$$\ntest";
my $Can_mkdir_With_Newline = mkdir $Test_Dir;

Finally the fact that the thrown error is not the one from File::chdir::_chdir AND that it mentions the directory without a newline, tells me that it is Perl throwing the error and most like that it is its chdir function for Cygwin doing it. Note that the error should be:

Cannot chdir back to /home/maurice/projects/File-chdir-0.1007/t/testdir3492
test: No such file or directory at /home/maurice/projects/File-chdir-0.1007/blib/lib/File/chdir.pm line 21.
@jberger
Member
jberger commented Dec 1, 2012

I believe this is a bug in Perl!

  1. On cygwin abs_path is mapped to fast_abs_path for some reason. We can also know that this is true, because this is where that specific text of the error is found.
  2. Unlike File::chdir, Cwd::fast_abs_path untaints without the /s flag, truncating any directories with newlines in them.

(Strangely I think I am proposing that it is File::chdir::Scalar::FETCH throwing this error, not STORE (though it may be both)! Again the real bug seems to be in Cwd::fast_abs_path)

Do you all concur? Should I file a bug on Perl?

@jberger
Member
jberger commented Dec 1, 2012

@mokko can you test something on cygwin? Not using Perl, create a directory with a newline in it and cd into it. Then run

perl -MCwd -e "print Cwd::abs_path"

If that dies we have a bug in Perl.

@jberger
Member
jberger commented Dec 1, 2012

Sorry for lots of noise, but of course I can try it on linux and just use the fast_abs_path and indeed it dies

$ mkdir 'hello
world'
$ cd 'hello
world'
$ perl -MCwd -E 'say Cwd::fast_abs_path'
Cannot chdir back to /home/joel/Desktop/test/hello: No such file or directory at -e line 1
@mokko
mokko commented Dec 1, 2012

I don't understand all details, but I get the gist. As you suspected, perl on cygwin returns
Cannot chdir back to /home/maurice/directory: No such file or directory at -e line 1.
when the oneliner is executed from with directory with newline.

@jberger
Member
jberger commented Dec 1, 2012

Filed bug on Perl: RT#115962

@mokko
mokko commented Dec 3, 2012

sounds plausible to me. It doesn't resolve the test failure, though. We need to change t/newline.t also to use getcwd instead of abs_path. Do you want me to provide a PR? Or do you want to change it yourself, dagoldern? It's only one pretty obvious line.

@jberger
Member
jberger commented Dec 3, 2012

(repeated for clarity) My full pull request has addressed these concerns: #4

@dagolden
dagolden commented Dec 3, 2012

Fixed and released to CPAN.

@dagolden dagolden closed this Dec 3, 2012
@schwern schwern pushed a commit to evalEmpire/piledriver that referenced this issue Jan 1, 2014
@jberger jberger + Father Chrysostomos Cwd::fast_abs_path's untaint should allow for multiline directories
This bug was noticed via Perl-Toolchain-Gang/File-chdir#3 and testing has led to this being the cause.
The problem is worse on some platforms (notably cygwin in this case) when abs_path is implemented by fast_abs_path.
Since File::chdir tests for proper behavior when a directory contains a newline, this bug then breaks File::chdir (one of my favorites and very useful xplatform tool).

Yes this should have tests, but since it will involve creating a directory with a newline, I thought I would do better to leave that to someone with better knowledge than I.
9f28c63
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment