Closed ChrisBaker97 closed 3 years ago
sahib
commented
3 years ago Hmm, this seems to be a strange behavior difference between Linux and macOS (and BSDs). I will check (hopefully tomorrow) if we can live without that flag. Thanks for the PR. :+1:
sahib
commented
3 years ago Closed by accident, pardon.
sahib
commented
3 years ago So, here's the issue:
$ echo x > /tmp/a
$ echo x > /tmp/b
$ ln -s /tmp/a /tmp/la
$ ln -s /tmp/b /tmp/lb
$ rm /tmp/{a,b}
$ ./rmlint-with-flag /tmp/l{a,b}
# Bad symlink(s):
rm '/tmp/la'
rm '/tmp/lb'
$ ./rmlint-without-flag /tmp/l{a,b,}
WARNING: Can't open directory or file "/tmp/la": No such file or directory
WARNING: Can't open directory or file "/tmp/lb": No such file or directory
ERROR: Not all given paths are valid. Aborting.This is expected since faccesat() will fail on a broken symbolic link without this flag. Seems we need to check if it is a symbolic link if faccessat() errors out...
ChrisBaker97
commented
3 years ago I don't think I'll be able to contribute anything else to the coding, but I'm happy to help with any testing. Feel free to modify this PR as desired.
ericonr
commented
3 years ago Using readlink() might be a good solution here (note that I haven't looked into the code properly, the thought just crossed my mind).
ericonr
commented
3 years ago Ooh, this is a complicated one. I believe musl and glibc implement faccessat quite differently, where musl implements a workaround for correctness (probably approaches the result of the BSDs / MacOS) whereas glibc doesn't. That's my understanding, at least.
ericonr
commented
3 years ago Would it be possible to simply check if the file can be opened at a later point? Using access is racy and the behavior between the versions using faccessat and access is different.
ericonr
commented
3 years ago Lastly, if anything this should completely revert the commit, right? 5ca6d25905cd372e8b07ecd9696a097acb0d5742
sahib
commented
3 years ago Took me again a bit longer to come back to this, but I should have a fix with a726d0cb. @ericonr, @ChrisBaker97 - please test that one.
Ooh, this is a complicated one. I believe musl and glibc implement faccessat quite differently, where musl implements a workaround for correctness (probably approaches the result of the BSDs / MacOS) whereas glibc doesn't. That's my understanding, at least.
Yes, this seems to be the case.
Using readlink() might be a good solution here (note that I haven't looked into the code properly, the thought just crossed my mind).
That is indeed the solution I did go for now, but lstat() would have worked too.
Would it be possible to simply check if the file can be opened at a later point? Using access is racy and the behavior between the versions using faccessat and access is different.
That check is there to filter files out early that we can't open at all. There are other checks later on.
ChrisBaker97
commented
3 years ago Seems to compile and run fine on macOS 10.14.6 Mojave. Thanks for the fix.
ericonr
commented
3 years ago Took me again a bit longer to come back to this, but I should have a fix with a726d0c. @ericonr, @ChrisBaker97 - please test that one.
It appears to be working, though I don't have much experience with the tool.
Thank you for the explanations :)
ericonr
commented
3 years ago That is indeed the solution I did go for now, but lstat() would have worked too.
As a general heads up, you shouldn't use lstat() here either. At least for both glibc and musl, they are implemented as simple wrappers on top of faccessat().
ChrisBaker97
commented
3 years ago FYI, I just ran a partial test suite on just the new test added, using:
RM_TS_USE_VALGRIND=1 RM_TS_PRINT_CMD=1 RM_TS_PEDANTIC=1 nosetests-3.4 -s -a '!slow !known_issue' tests/test_robustness/test_badlinks_as_args.pywith the result:
======================================================================
FAIL: test_badlinks_as_args.test_bad_symlinks_as_direct_args
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/chris/.pyenv/versions/3.4.10/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/Users/chris/Code/rmlint/tests/test_robustness/test_badlinks_as_args.py", line 33, in test_bad_symlinks_as_direct_args
{link_a_path, link_b_path}
AssertionError
----------------------------------------------------------------------
Ran 3 tests in 3.381s
FAILED (failures=1)I also ran what looks to be an existing symlink test:
$ RM_TS_USE_VALGRIND=1 RM_TS_PRINT_CMD=1 RM_TS_PEDANTIC=1 nosetests-3.4 -s -a '!slow !known_issue' tests/test_options/test_symlinks.pyand this was the result:
======================================================================
FAIL: test_symlinks.test_default
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/chris/.pyenv/versions/3.4.10/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/Users/chris/Code/rmlint/tests/test_options/test_symlinks.py", line 21, in test_default
'/a/z',
AssertionError
======================================================================
FAIL: test_symlinks.test_merge_directories_with_ignored_symlinks
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/chris/.pyenv/versions/3.4.10/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/Users/chris/Code/rmlint/tests/test_options/test_symlinks.py", line 36, in test_merge_directories_with_ignored_symlinks
'/b',
AssertionError
----------------------------------------------------------------------
Ran 5 tests in 27.876s
FAILED (failures=2)I wasn't sure whether nosetests-3.4 was testing against Python 3.4, which is why I ran it using Python 3.4.10, but I'm also getting the same results with 3.8.6 and 3.9.0 as well.
sahib
commented
3 years ago As a general heads up, you shouldn't use lstat() here either. At least for both glibc and musl, they are implemented as simple wrappers on top of faccessat().
@ericonr : You probably mean as wrappers to fstatat()? I actually tested that variant and seemed to work.
FYI, I just ran a partial test suite on just the new test added, using:
I just tried to reproduce this behavior in an alpine docker container, but couldn't. Could it be that you have a old rmlint binary lying around?
ChrisBaker97
commented
3 years ago FYI, I just ran a partial test suite on just the new test added, using:
I just tried to reproduce this behavior in an alpine docker container, but couldn't. Could it be that you have a old
rmlintbinary lying around?
which rmlint does, in fact, return my Homebrew-installed rmlint. I'm not familiar enough with nosetest to know if there's a better way to do this, but I just aliased rmlint to the version compiled from the latest develop branch, ran the tests again, and got the same result:
Again, run with Python 3.4, but same results in 3.9.0.
sahib
commented
3 years ago which rmlint does, in fact, return my Homebrew-installed rmlint. I'm not familiar enough with nosetest to know if there's a better way to do this, but I just aliased rmlint to the version compiled from the latest develop branch, ran the tests again, and got the same result:
alias won't help since it's seen only by your shell. nosetests simply picks the rmlint in the directory you're executing it from. But /Users/chris/Code/rmlint/rmlint already has the latest fix, judging from the --version output. Weird.
Again, run with Python 3.4, but same results in 3.9.0.
Python version probably does not matter here.
Still no luck reproducing that issue in my alpine container (which uses musl as libc, like other reporters). Maybe there's another issue on macOS' libc?
Seems to compile and run fine on macOS 10.14.6 Mojave. Thanks for the fix.
You did execute the test in https://github.com/sahib/rmlint/pull/444#issuecomment-727606928 and it worked (files were detected as broken symlinks), right? Also did the above tests also fail with versions that did not have the fix yet? The last test (which I added recently) is especially weird since it fails only at the last check, indicating that there were some files detected as bad links.
ChrisBaker97
commented
3 years ago Gotcha. Yeah, I'm running it from the base directory of the repository, which is where rmlintgets built, so it looks like it's testing with the modified version either way.
You did execute the test in #444 (comment) and it worked (files were detected as broken symlinks), right? Also did the above tests also fail with versions that did not have the fix yet?
I didn't but here's the output. Looks like the modified version works as expected, while the stock 2.10.1 version doesn't:
$ echo x > /tmp/a > /tmp/b
$ ln -s /tmp/a /tmp/la
$ ln -s /tmp/b /tmp/lb
$ rm -f /tmp/{a,b}
$ ./rmlint --version
version 2.10.1 compiled: Dec 18 2020 at [14:17:10] "Ludicrous Lemur" (rev ef12a601)
compiled with: -mounts -nonstripped -fiemap +sha512 +bigfiles +intl +replay +xattr -btrfs-support
rmlint was written by Christopher <sahib> Pahl and Daniel <SeeSpotRun> Thomas.
The code at https://github.com/sahib/rmlint is licensed under the terms of the GPLv3.
$ ./rmlint /tmp/l{a,b}
# Bad symlink(s):
rm '/tmp/lb'
rm '/tmp/la'
==> Note: Please use the saved script below for removal, not the above output.
==> In total 2 files, whereof 0 are duplicates in 0 groups.
==> This equals 0 B of duplicates which could be removed.
==> 2 other suspicious item(s) found, which may vary in size.
==> Scanning took in total 0.054s.
Wrote a sh file to: /Users/chris/Code/rmlint/rmlint.sh
Wrote a json file to: /Users/chris/Code/rmlint/rmlint.json
$ /usr/local/bin/rmlint --version
version 2.10.1 compiled: Nov 12 2020 at [20:13:37] "Ludicrous Lemur" (rev unknown)
compiled with: -mounts -nonstripped -fiemap +sha512 +bigfiles +intl +replay +xattr -btrfs-support
rmlint was written by Christopher <sahib> Pahl and Daniel <SeeSpotRun> Thomas.
The code at https://github.com/sahib/rmlint is licensed under the terms of the GPLv3.
$ /usr/local/bin/rmlint /tmp/l{a,b}
WARNING: Can't open directory or file "/tmp/la": No such file or directory
WARNING: Can't open directory or file "/tmp/lb": No such file or directory
ERROR: Not all given paths are valid. Aborting.(I should mention that the "stock" version is built after applying the patch I submitted to Homebrew's formula, deleting |AT_SYMLINK_NOFOLLOW from cfg.c as shown in the initial commit to this pull request, which was necessary to get it working at all on macOS 10.14.x Mojave and older after 2.9.0. If that's a problem, let me know, and I'll roll back to 2.9.0 and test again.)
sahib
commented
3 years ago Thanks @ChrisBaker97. I pushed a small commit with some added prints in the tests. Can you please re-run the problematic tests so we what's going on?
ChrisBaker97
commented
3 years ago Here you go:
$ git fetch origin && git reset --hard origin/develop$ scons config --without-gui && scons DEBUG=1$ ./rmlint --version$ RM_TS_USE_VALGRIND=1 RM_TS_PRINT_CMD=1 RM_TS_PEDANTIC=1 nosetests-3.4 -s -a '!slow !known_issue' tests/test_options/test_symlinks.py tests/test_robustness/test_badlinks_as_args.pysahib
commented
3 years ago Okay, thanks. Good news, it's just a error in the test :smile: The paths get cut off incorrectly on macOS, probably because rmlint followed a link (which shows in the path, ). Will fix tomorrow.
Out of curiosity, do the test work when you do this?
$ RM_TS_DIR=/private/tmp/rmlint-unit-testdir/ RM_TS_PRINT_CMD=1 nosetests-3.4 -d -s -a '!slow !known_issue' tests/test_options/test_symlinks.py tests/test_robustness/test_badlinks_as_args.py...also I suppose those are not the only tests that fail on macOS?
sahib
commented
3 years ago I should have noted: The -d above makes nose output the values it tried to assert, which is helpful during debugging.
sahib
commented
3 years ago 17ada44f contains a fix that should do the trick for those tests.
ChrisBaker97
commented
3 years ago Out of curiosity, do the test work when you do this?
$ RM_TS_DIR=/private/tmp/rmlint-unit-testdir/ RM_TS_PRINT_CMD=1 nosetests-3.4 -d -s -a '!slow !known_issue' tests/test_options/test_symlinks.py tests/test_robustness/test_badlinks_as_args.py...also I suppose those are not the only tests that fail on macOS?
You know, I've tried to run the full test suite a couple of times, but always run out of patience after 30 mins or so. Pretty deep into it, IIRC, it prints a line F.. and then seems to hang longer than I care to wait. The system is still loaded from it, and I end up having to close out the terminal.
We can work on that next, if you want. 😊
17ada44 contains a fix that should do the trick for those tests.
Looks good, I think.
$ git fetch origin && git reset --hard origin/develop$ scons config --without-gui && scons DEBUG=1$ ./rmlint --version$ RM_TS_USE_VALGRIND=1 RM_TS_PRINT_CMD=1 RM_TS_PEDANTIC=1 nosetests-3.4 -d -s -a '!slow !known_issue' tests/test_options/test_symlinks.py tests/test_robustness/test_badlinks_as_args.pysahib
commented
3 years ago You know, I've tried to run the full test suite a couple of times, but always run out of patience after 30 mins or so. Pretty deep into it, IIRC, it prints a line F.. and then seems to hang longer than I care to wait. The system is still loaded from it, and I end up having to close out the terminal.
Welp, interesting. Does that still happen if you just do:
# That takes about 60s on my rather slow laptop:
$ nosetests -d -s -a '!slow' testsJust for background:
RM_TS_USE_VALGRIND=1: will run each test in valgrind, which is very slowRM_TS_PEDANTIC=1: Runs each test several times with different base options.-a '!slow !knownissue apparently does not work. It should be -a '!slow' -a '!knownissue', but since we don't have knownissue tags the first part is enough.Looks good, I think.
Nice.
ChrisBaker97
commented
3 years ago That completed at least. Look's like Apple's touch doesn't like -d.
$ nosetests -d -s -a '!slow' testsI had been using RM_TS_USE_VALGRIND=1 RM_TS_PRINT_CMD=1 RM_TS_PEDANTIC=1 nosetests-3.4 -s -a '!slow !known_issue' to run all tests before. (Not sure if leaving off tests at the end was causing it to pick up anything outside of the tests directory as well.)
ChrisBaker97
commented
3 years ago A little over an hour later...
$ RM_TS_USE_VALGRIND=1 RM_TS_PRINT_CMD=1 RM_TS_PEDANTIC=1 nosetests-3.4 -d -s -a '!slow !known_issue' testsFull output, if you need it.
sahib
commented
3 years ago Thanks @ChrisBaker97,
some errors happen because you're missing some test dependencies (parametrized for example). You can install them with pip3 install -r test-requirements.txt. Some of the other errors need more debugging - can't tell if they hint at bugs. Would be good if somebody with macOS could take a look and make a PR eventually.
But I think the original cause of this issue has been resolved, so I will close that ticket here. Thanks all.
This reversion back to the code from version
2.9.0addresses #438. Obviously there is some functionality here that's being implemented that I'm not familiar enough with the project to understand, but hopefully this gives someone a head start in trying to track down the bug, while still keeping the intended new behavior.