$! not set properly for unlink on < 5.18 #78
Comments
|
I wonder if we would see the same issue if we decide to build Perl on more recent version of Debian container. Note that |
|
@toddr thanks for the report, let me check further shortly 💪 Note that those using docker-perl images before the currently supported perlpolicy versions are advised to rebuild and customize their own images using the Dockerfiles here in this repo as needed (note to self: probably should note this down in the stretch tag.
@atoomic for supported Perls we default on |
|
This may relate to the fact that Perl doesn’t actually unlink() a directory because some OSes actually allow this, even though it corrupts the filesystem. |
|
Running under # after installdeps for Test::[email protected]...
root@545808d206c3:~# SHELL=/bin/bash cpanm --look Test::[email protected]
--> Working on Test::MockFile
Fetching http://backpan.perl.org/authors/id/T/TO/TODDR/Test-MockFile-0.020.tar.gz ... OK
Entering /root/.cpanm/work/1580471299.2635/Test-MockFile-0.020 with /bin/bash
root@545808d206c3:~/.cpanm/work/1580471299.2635/Test-MockFile-0.020# prove -lv t/touch.t
t/touch.t ..
# Seeded srand with seed '20200131' from local date.
# -------------- REAL MODE --------------
ok 1 - unlink on a dir fails
# -------------- MOCK MODE --------------
ok 2 - unlink /link works.
ok 3 - /link is now gone
ok 4 - unlink /dir doesn't work.
not ok 5 - ... and throws a $!
# Failed test ' ... and throws a $!'
# at t/touch.t line 33.
# +-----+----+-------+
# | GOT | OP | CHECK |
# +-----+----+-------+
# | 21 | eq | 0 |
# +-----+----+-------+
ok 6 - touch /dir doesn't work.
ok 7 - touch /link doesn't work.
ok 8 - Set mtime to 1970
ok 9 - Set ctime to 1970
ok 10 - Set atime to 1970
ok 11 - Touch a missing file.
ok 12 - mtime is set.
ok 13 - ctime is set.
ok 14 - atime is set.
ok 15 - /file exists with -e
ok 16 - /file is removed via unlink method
ok 17 - /file is missing via contents check
ok 18 - /file is missing via size method
ok 19 - /file is removed via -e check
ok 20 - Set file to have stuff in it.
ok 21 - Touch an existing file.
ok 22 - mtime is set to 1234.
ok 23 - ctime is set to 1234.
ok 24 - atime is set to 1234.
1..24
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/24 subtests
Test Summary Report
-------------------
t/touch.t (Wstat: 256 Tests: 24 Failed: 1)
Failed test: 5
Non-zero exit status: 1
Files=1, Tests=24, 0 wallclock secs ( 0.03 usr 0.01 sys + 0.43 cusr 0.04 csys = 0.51 CPU)
Result: FAILLet me check with a rebuild of 5.18 against |
|
@FGasper is probably right, it is noted in perldoc after all:
That last bit probably warrants a perlport note as well. |
|
Heh, and current the 5.18 build will still fail |
|
@zakame Could we consider updating the debian release Do you see any issues updating the containers this way? |
|
@atoomic for the unsupported perls from 5.24 down, we no longer automatically publish updates; only those listed in https://github.com/docker-library/official-images/blob/master/library/perl get rebuilt by docker-library's Jenkins pipeline (see also https://github.com/docker-library/oi-janky-groovy.) The main issue is the sheer size; for these older perls, we'd be asking docker-library to rebuild around 24 images, yet not all of them are guaranteed to pass building (we still have the Time-Local test issue to fix/disable, as well as perhaps other things to check.) Again, I'd encourage users/orgs to fork and customize the Dockerfiles here especially for running older Perls on newer OS releases (note that one doesn't have to be limited to Debian, they can use |
|
Back to @toddr's original issue, it seems that even when rebuilding to I suspect @FGasper is right on this one and we should be testing for file type prior to |
The point of the test is to confirm a consistent behavior of the test vs the operating system it is on. I would be shocked if you could show me that this particular failure to set In my case I did test for the existence of the directory and it was there so |
I wouldn't be shocked if this would happen on any container/OS combination. There are also subtle differences in syscall or other behavior depending when its on a container or not, see for example #44. FWIW this issue seems confirmed to be present on jessie and stretch containers but not on buster: so I suppose that's as good a lead as any to trace this issue. In particular, I note https://metacpan.org/source/TODDR/Test-MockFile-0.020/t/touch.t#L17 being done without any testing for superuser or capabilities at all; given a container may have superuser-like capabilities on a non-root to run processes with (or go the opposite, have |
|
FWIW, our RHEL/OEL6 systems use 5.10.1 our RHEL/OEL7 systems use 5.16.3, another system I have is 5.30.3 and every single one of them sees '21' rather than 0. Are we sure the unit test is not implicitly assuming something else about it's runtime environment? |
|
I continue to believe there is something wrong with the docker images. I don't see how this should ever not be an error. |
You hit the dot with root: Yields: |
|
None of that is needed. Just try this simplified example provided by @clayne. #78 (comment) we should be getting 21 always but it appears the older perl docker containers do not behave as expected.
|
|
|
Hi @toddr, @waterkip, and @clayne, thanks for the updates, I can replicate this certainly with the simple test: So I think that while you might be checking for the directory existence, you lack checking for whether you're root or not, precisely as what the perldoc in #78 (comment) warns about. @clayne's insight about assumptions on runtime is apt; FWIW, here's the same pattern when running under Kubernetes, emphasizing the lack of From the PoV of a Test::MockFile user, I'd suggest letting https://metacpan.org/pod/Test::MockFile#unlink be consistent with what's documented in the perlfunc and not support unlinking directories; you're supposed to be mocking existing (if albeit unreliable) behavior after all, not deviating from it. |
|
As a follow up, I tested again with 5.32 on jessie, and it is consistent with the original observation for Again, all of this simply points out the fact that |
|
This was a bug in perl that was fixed in 1dcae8b8dd1e2aa373ab045fee3d4f95d34f0b3c |
I found a bug today switching Test::MockFile over to github actions.
https://github.com/cpanel/Test-MockFile/runs/418056154?check_suite_focus=true
You can see the test failure here:
The issue appears to be that $! is not set when unlink is tried on a directory.
I simplified this problem to do:
mkdir -p /tmp/foo; ls -ld /tmp/foo; perl -E'CORE::unlink "/tmp/foo"; print $! + 0; print "\n"'; ls -ld /tmp/fooOn everything above 5.18, I get:
On everything at and below 5.18, I get:
For whatever reason, perl is broken subtly on the older perls OR jesse is a problem?
The text was updated successfully, but these errors were encountered: