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

Fix temporary directory handling in serializer API test #1133

Merged
merged 1 commit into from Oct 26, 2015

Conversation

nanis
Copy link
Contributor

@nanis nanis commented Oct 19, 2015

Even on those systems where the test seemingly succeeds, this test
file does not respect the user's configured temporary directory.

The test file t/14_serializer/06_api.t contains a misleading comment:

Ensure the temp directory is resolved before we destroy the

environment (since it needs the environment to do so, and

caches after the first time)

File::Spec->tmpdir;

which implies that the author thought the cached tmpdir value would be
preserved even when all the environment variables used by
File::Spec->tmpdir are purged from %ENV.

This assumption is incorrect.

An inspection of File::Spec::Unix shows clearly that the cached
value of tmpdir is invalidated in this instance. This happens on all
platforms.

On *nix systems, File::Spec->tmpdir falls back to using /tmp if it
cannot resolve a temporary directory using $ENV{TMPDIR}. This enables
the tests to succeed, but the tests do not used the supposedly cached
tmpdir from the call above. They use /tmp.

Looking at File::Spec::Win32, we note that tmpdir tries to fall
back to using one of the following hardcoded locations if it cannot resolve
a temporary directory location from commonly used environment variables:
SYS:/temp, C:\system\temp, C:/temp, /tmp, and /.

Modern Windows systems do not contain any of these directories out of the
box. Therefore, the value cached by File::Spec->tmpdir is invalidated
when the relevant environment variables are removed from %ENV.

Consequently, it returns / as the temporary directory
location which, on the system I am using to test CPAN modules, results in
the following test failure:

Error in tempfile() using template \XXXXXXXXXX: Could not create temp file
\IpjtLLGiT3: Permission denied at
C:/opt/perl-5.22.0/site/lib/HTTP/Body/OctetStream.pm line 33.
# Looks like you planned 18 tests but ran 15.
# Looks like your test exited with 13 just after 15.
t\14_serializer\06_api.t ............................
Dubious, test returned 13 (wstat 3328, 0xd00)
Failed 3/18 subtests

On other systems, the problem may be hidden by the fact that /tmp is there,
or maybe because there is a C:\Temp, or maybe because the root directory is
writable by anyone etc.

Even on those systems where the test seemingly succeeds, this test
file does not respect the user's configured temporary directory.

The solution is to include TMPDIR in the reduced %ENV hash. $ENV{TMPDIR}
is respected by File::Spec on all OS it supports.

*Even on those systems where the test seemingly succeeds, this test
file does not respect the user's configured temporary directory.*

The test file t/14_serializer/06_api.t contains a [misleading comment][3]:

> # Ensure the temp directory is resolved before we destroy the
> # environment (since it needs the environment to do so, and
> # caches after the first time)
> File::Spec->tmpdir;

which implies that the author thought the cached tmpdir value would be
preserved even when all the environment variables used by
`File::Spec->tmpdir` are purged from `%ENV`.

This assumption is incorrect.

An inspection of [File::Spec::Unix][1] shows clearly that the cached
value of `tmpdir` is invalidated in this instance. This happens on all
platforms.

On *nix systems, `File::Spec->tmpdir` falls back to using `/tmp` if it
cannot resolve a temporary directory using `$ENV{TMPDIR}`. This enables
the tests to succeed, but the tests do not used the supposedly cached
`tmpdir` from the call above. They use `/tmp`.

Looking at [File::Spec::Win32][2], we note that `tmpdir` tries to fall
back to using one of the following hardcoded locations if it cannot resolve
a temporary directory location from commonly used environment variables:
`SYS:/temp`, `C:\system\temp`, `C:/temp`, `/tmp`, and `/`.

Modern Windows systems do not contain any of these directories out of the
box. Therefore, the value cached by `File::Spec->tmpdir` is invalidated
when the relevant environment variables are removed from `%ENV`.

Consequently, it returns `/` as the temporary directory
location which, on the system I am using to test CPAN modules,  results in
the following test failure:

    Error in tempfile() using template \XXXXXXXXXX: Could not create temp file
    \IpjtLLGiT3: Permission denied at
    C:/opt/perl-5.22.0/site/lib/HTTP/Body/OctetStream.pm line 33.
    # Looks like you planned 18 tests but ran 15.
    # Looks like your test exited with 13 just after 15.
    t\14_serializer\06_api.t ............................
    Dubious, test returned 13 (wstat 3328, 0xd00)
    Failed 3/18 subtests

On other systems, the problem may be hidden by the fact that `/tmp` is there,
or maybe because there is a `C:\Temp`, or maybe because the root directory is
writable by anyone etc.

*Even on those systems where the test seemingly succeeds, this test
file does not respect the user's configured temporary directory.*

The solution is to include `TMPDIR` in the reduced `%ENV` hash. `$ENV{TMPDIR}`
is respected by `File::Spec` on all OS it supports.

 [1]: https://metacpan.org/source/SMUELLER/PathTools-3.47/lib/File/Spec/Unix.pm#L207
 [2]: https://metacpan.org/source/SMUELLER/PathTools-3.47/lib/File/Spec/Win32.pm#L70
 [3]: https://github.com/PerlDancer/Dancer/blob/devel/t/14_serializer/06_api.t#L58
@nanis
Copy link
Contributor Author

nanis commented Oct 19, 2015

I see the Travis CI builds failed, but I don't think the failures are related to the patch. All the builds fail with:

[DZ] writing Dancer in /home/travis/build/PerlDancer/Dancer/build_dir
fatal: Not a valid object name undefined
The command "source ~/travis-perl-helpers/init --auto" failed and exited with 128 during .

@nanis
Copy link
Contributor Author

nanis commented Oct 19, 2015

Examples of File::Spec->tmpdir's cached value being invalidated when %ENV is cleared.

On Linux:

$ perl -MFile::Spec -E 'say File::Spec->tmpdir; %ENV = (); say File::Spec->tmpdir`
/home/user/tmp
/tmp

On OS X:

$ perl -MFile::Spec -E 'say File::Spec->tmpdir; %ENV = (); say File::Spec->tmpdir`
/var/folders/fm/.../T
/tmp

On Windows 8.1

C:\> perl -MFile::Spec -E "say File::Spec->tmpdir; %ENV=(); say File::Spec->tmpdir"
C:\Users\...\AppData\Local\Temp
\

@bigpresh
Copy link
Member

Thanks for this - will get it merged soon and work out what's wrong with Travis. I know Travis builds were failing until recently, so it may be just that you need to merge devel into your branch (or I'll check out your PR as a branch, merge from devel, and raise a new PR)

@nanis
Copy link
Contributor Author

nanis commented Oct 21, 2015

Thank you for your work.

@bigpresh bigpresh merged commit d017587 into PerlDancer:devel Oct 26, 2015
@nanis nanis deleted the nanis-fix-tmpfile-test-fail branch October 27, 2015 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants