* [PATCH] Fix some failing tests in BSD systems [not found] <m1sg7i7xfl.fsf.ref@yahoo.es> @ 2021-01-03 17:16 ` Daniel Martín 2021-01-03 17:35 ` Eli Zaretskii 2021-01-04 1:43 ` Dmitry Gutov 0 siblings, 2 replies; 16+ messages in thread From: Daniel Martín @ 2021-01-03 17:16 UTC (permalink / raw) To: emacs-devel [-- Attachment #1: Type: text/plain, Size: 916 bytes --] A couple of tests are failing for me on master and a BSD system (macOS): 2 unexpected results: FAILED xref--xref-file-name-display-is-abs FAILED xref--xref-file-name-display-is-relative-to-project-root An extract of a test failure is (list-elt 0 (arrays-of-different-length 25 24 "xref-resources//file1.txt" "xref-resources/file1.txt" first-mismatch-at 15)) The reason for the failure is that BSD 'find' sometimes outputs paths with an extra '/', for example: find /path/emacs/test/lisp/progmodes/xref-resources/ -type f \( -iname \* \) /path/emacs/test/lisp/progmodes/xref-resources//file2.txt /path/emacs/test/lisp/progmodes/xref-resources//file1.txt (Note the trailing '/' in the first argument to 'find'.) As I think this differs from GNU find but it's still a valid POSIX path, I decided to make the tests more flexible. Could you install the attached patch if you think it does TRT? Thanks. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-some-xref-tests-for-systems-with-BSD-find.patch --] [-- Type: text/x-patch, Size: 3288 bytes --] From 6c82722c7ecba28fce619fa4b4fcb3d4425ffa64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es> Date: Sun, 3 Jan 2021 18:13:07 +0100 Subject: [PATCH] Fix some xref-tests for systems with BSD find Some versions of BSD find, like those present on macOS systems, may return a file path with two intermediate slashes (eg. "path/to//resource"). As this is valid POSIX, just make the tests more flexible to accommodate for this situation. --- test/lisp/progmodes/xref-tests.el | 38 +++++++++++++++++++------------ 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/test/lisp/progmodes/xref-tests.el b/test/lisp/progmodes/xref-tests.el index eaafc5888c..dd083d655b 100644 --- a/test/lisp/progmodes/xref-tests.el +++ b/test/lisp/progmodes/xref-tests.el @@ -99,13 +99,18 @@ xref--buf-pairs-iterator-cleans-up-markers (should (null (marker-position (cdr (nth 0 (cdr cons2)))))))) (ert-deftest xref--xref-file-name-display-is-abs () - (let ((xref-file-name-display 'abs)) - (should (equal (delete-dups - (mapcar 'xref-location-group - (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) - (list - (concat xref-tests--data-dir "file1.txt") - (concat xref-tests--data-dir "file2.txt")))))) + (let ((xref-file-name-display 'abs) + ;; BSD find may add an extra '/' to the path. + (expected (list + (concat xref-tests--data-dir "/?file1.txt") + (concat xref-tests--data-dir "/?file2.txt"))) + (actual (delete-dups + (mapcar 'xref-location-group + (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))))) + (should (and (= (length expected) (length actual)) + (cl-every (lambda (e1 e2) + (string-match-p e1 e2)) + expected actual))))) (ert-deftest xref--xref-file-name-display-is-nondirectory () (let ((xref-file-name-display 'nondirectory)) @@ -121,10 +126,15 @@ xref--xref-file-name-display-is-relative-to-project-root (file-name-directory (directory-file-name xref-tests--data-dir))) (project-find-functions #'(lambda (_) (cons 'transient data-parent-dir))) - (xref-file-name-display 'project-relative)) - (should (equal (delete-dups - (mapcar 'xref-location-group - (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) - (list - "xref-resources/file1.txt" - "xref-resources/file2.txt"))))) + (xref-file-name-display 'project-relative) + ;; BSD find may add an extra '/' to the path. + (expected (list + "xref-resources//?file1.txt" + "xref-resources//?file2.txt")) + (actual (delete-dups + (mapcar 'xref-location-group + (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))))) + (should (and (= (length expected) (length actual)) + (cl-every (lambda (e1 e2) + (string-match-p e1 e2)) + expected actual))))) -- 2.28.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-03 17:16 ` [PATCH] Fix some failing tests in BSD systems Daniel Martín @ 2021-01-03 17:35 ` Eli Zaretskii 2021-01-03 21:08 ` Daniel Martín 2021-01-04 1:41 ` Dmitry Gutov 2021-01-04 1:43 ` Dmitry Gutov 1 sibling, 2 replies; 16+ messages in thread From: Eli Zaretskii @ 2021-01-03 17:35 UTC (permalink / raw) To: Daniel Martín; +Cc: emacs-devel > From: Daniel Martín <mardani29@yahoo.es> > Date: Sun, 03 Jan 2021 18:16:46 +0100 > > (ert-deftest xref--xref-file-name-display-is-abs () > - (let ((xref-file-name-display 'abs)) > - (should (equal (delete-dups > - (mapcar 'xref-location-group > - (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) > - (list > - (concat xref-tests--data-dir "file1.txt") > - (concat xref-tests--data-dir "file2.txt")))))) > + (let ((xref-file-name-display 'abs) > + ;; BSD find may add an extra '/' to the path. > + (expected (list > + (concat xref-tests--data-dir "/?file1.txt") > + (concat xref-tests--data-dir "/?file2.txt"))) Why are we producing file names by concatenation, instead of calling expand-file-name? Wouldn't using the latter solve the problem? Or what did I miss? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-03 17:35 ` Eli Zaretskii @ 2021-01-03 21:08 ` Daniel Martín 2021-01-04 1:41 ` Dmitry Gutov 1 sibling, 0 replies; 16+ messages in thread From: Daniel Martín @ 2021-01-03 21:08 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel [-- Attachment #1: Type: text/plain, Size: 1114 bytes --] Eli Zaretskii <eliz@gnu.org> writes: >> From: Daniel Martín <mardani29@yahoo.es> >> Date: Sun, 03 Jan 2021 18:16:46 +0100 >> >> (ert-deftest xref--xref-file-name-display-is-abs () >> - (let ((xref-file-name-display 'abs)) >> - (should (equal (delete-dups >> - (mapcar 'xref-location-group >> - (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) >> - (list >> - (concat xref-tests--data-dir "file1.txt") >> - (concat xref-tests--data-dir "file2.txt")))))) >> + (let ((xref-file-name-display 'abs) >> + ;; BSD find may add an extra '/' to the path. >> + (expected (list >> + (concat xref-tests--data-dir "/?file1.txt") >> + (concat xref-tests--data-dir "/?file2.txt"))) > > Why are we producing file names by concatenation, instead of calling > expand-file-name? Wouldn't using the latter solve the problem? Or > what did I miss? Yes. I've attached another solution in terms of expand-file-name and file-relative-name. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Fix-some-xref-tests-for-systems-with-BSD-find.patch --] [-- Type: text/x-patch, Size: 2608 bytes --] From 4a6eabf615d6b82d4dc570afe801833dd03295ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es> Date: Sun, 3 Jan 2021 21:49:22 +0100 Subject: [PATCH] Fix some xref-tests for systems with BSD find Some versions of BSD find, like those present on macOS systems, may return a file path with two intermediate slashes (eg. "path/to//resource"). As this is valid POSIX, just make the tests build their paths in a more portable way. --- test/lisp/progmodes/xref-tests.el | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/lisp/progmodes/xref-tests.el b/test/lisp/progmodes/xref-tests.el index eaafc5888c..32cb2960a1 100644 --- a/test/lisp/progmodes/xref-tests.el +++ b/test/lisp/progmodes/xref-tests.el @@ -101,11 +101,13 @@ xref--buf-pairs-iterator-cleans-up-markers (ert-deftest xref--xref-file-name-display-is-abs () (let ((xref-file-name-display 'abs)) (should (equal (delete-dups - (mapcar 'xref-location-group - (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) + (mapcar + (lambda (e) + (expand-file-name (xref-location-group e))) + (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) (list - (concat xref-tests--data-dir "file1.txt") - (concat xref-tests--data-dir "file2.txt")))))) + (expand-file-name "file1.txt" xref-tests--data-dir) + (expand-file-name "file2.txt" xref-tests--data-dir)))))) (ert-deftest xref--xref-file-name-display-is-nondirectory () (let ((xref-file-name-display 'nondirectory)) @@ -121,10 +123,12 @@ xref--xref-file-name-display-is-relative-to-project-root (file-name-directory (directory-file-name xref-tests--data-dir))) (project-find-functions #'(lambda (_) (cons 'transient data-parent-dir))) - (xref-file-name-display 'project-relative)) + (xref-file-name-display 'project-relative)) (should (equal (delete-dups - (mapcar 'xref-location-group - (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) + (mapcar + (lambda (e) + (file-relative-name (xref-location-group e))) + (xref-tests--locations-in-data-dir "\\(bar\\|foo\\)"))) (list "xref-resources/file1.txt" "xref-resources/file2.txt"))))) -- 2.28.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-03 17:35 ` Eli Zaretskii 2021-01-03 21:08 ` Daniel Martín @ 2021-01-04 1:41 ` Dmitry Gutov 2021-01-04 3:30 ` Eli Zaretskii 1 sibling, 1 reply; 16+ messages in thread From: Dmitry Gutov @ 2021-01-04 1:41 UTC (permalink / raw) To: Eli Zaretskii, Daniel Martín; +Cc: emacs-devel On 03.01.2021 19:35, Eli Zaretskii wrote: > Why are we producing file names by concatenation, instead of calling > expand-file-name? Wouldn't using the latter solve the problem? Or > what did I miss? While we sometimes do that (expand-file-name is not free, and for certain uses its overhead counts), that doesn't seem to be the case here. The double slash here is produced by an external program, not by concatenation. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 1:41 ` Dmitry Gutov @ 2021-01-04 3:30 ` Eli Zaretskii 2021-01-04 13:10 ` Dmitry Gutov 0 siblings, 1 reply; 16+ messages in thread From: Eli Zaretskii @ 2021-01-04 3:30 UTC (permalink / raw) To: Dmitry Gutov; +Cc: emacs-devel, mardani29 > Cc: emacs-devel@gnu.org > From: Dmitry Gutov <dgutov@yandex.ru> > Date: Mon, 4 Jan 2021 03:41:20 +0200 > > On 03.01.2021 19:35, Eli Zaretskii wrote: > > Why are we producing file names by concatenation, instead of calling > > expand-file-name? Wouldn't using the latter solve the problem? Or > > what did I miss? > > While we sometimes do that (expand-file-name is not free, and for > certain uses its overhead counts), that doesn't seem to be the case here. Not sure what you mean: I clearly saw calls to concat in the patch, both in the old and the new code. What did I miss? > The double slash here is produced by an external program, not by > concatenation. I understand that much, but expand-file-name removes the double slashes as part of its job, doesn't it? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 3:30 ` Eli Zaretskii @ 2021-01-04 13:10 ` Dmitry Gutov 2021-01-04 15:52 ` Eli Zaretskii 0 siblings, 1 reply; 16+ messages in thread From: Dmitry Gutov @ 2021-01-04 13:10 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel, mardani29 On 04.01.2021 05:30, Eli Zaretskii wrote: >> Cc: emacs-devel@gnu.org >> From: Dmitry Gutov <dgutov@yandex.ru> >> Date: Mon, 4 Jan 2021 03:41:20 +0200 >> >> On 03.01.2021 19:35, Eli Zaretskii wrote: >>> Why are we producing file names by concatenation, instead of calling >>> expand-file-name? Wouldn't using the latter solve the problem? Or >>> what did I miss? >> >> While we sometimes do that (expand-file-name is not free, and for >> certain uses its overhead counts), that doesn't seem to be the case here. > > Not sure what you mean: I clearly saw calls to concat in the patch, > both in the old and the new code. What did I miss? The changes you are referring to are both in the test code. There's nothing inherently wrong in using concat in such code, as long as we test what we want to test. >> The double slash here is produced by an external program, not by >> concatenation. > > I understand that much, but expand-file-name removes the double > slashes as part of its job, doesn't it? Yes, so? If you actually meant to suggest an alternative comparison strategy (like in Daniel's latest patch), then I think the test becomes unnecessarily lax. After all, we want to also check how the string "looks" (because the user will see it verbatim), not just what file it points at. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 13:10 ` Dmitry Gutov @ 2021-01-04 15:52 ` Eli Zaretskii 2021-01-04 17:10 ` Dmitry Gutov 0 siblings, 1 reply; 16+ messages in thread From: Eli Zaretskii @ 2021-01-04 15:52 UTC (permalink / raw) To: Dmitry Gutov; +Cc: emacs-devel, mardani29 > Cc: mardani29@yahoo.es, emacs-devel@gnu.org > From: Dmitry Gutov <dgutov@yandex.ru> > Date: Mon, 4 Jan 2021 15:10:40 +0200 > > >> On 03.01.2021 19:35, Eli Zaretskii wrote: > >>> Why are we producing file names by concatenation, instead of calling > >>> expand-file-name? Wouldn't using the latter solve the problem? Or > >>> what did I miss? > >> > >> While we sometimes do that (expand-file-name is not free, and for > >> certain uses its overhead counts), that doesn't seem to be the case here. > > > > Not sure what you mean: I clearly saw calls to concat in the patch, > > both in the old and the new code. What did I miss? > > The changes you are referring to are both in the test code. There's > nothing inherently wrong in using concat in such code, as long as we > test what we want to test. There's nothing wrong with that, sure, but you risk false failures due to issues like the one here. However, if you are okay with that, I don't mind. It just was strange to see such code. > If you actually meant to suggest an alternative comparison strategy > (like in Daniel's latest patch), then I think the test becomes > unnecessarily lax. After all, we want to also check how the string > "looks" (because the user will see it verbatim), not just what file it > points at. I don't think you can reliably know how the file name will be formatted by a particular variety of 'find', not IME. But I'm not going to argue about this. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 15:52 ` Eli Zaretskii @ 2021-01-04 17:10 ` Dmitry Gutov 2021-01-04 22:25 ` Daniel Martín 0 siblings, 1 reply; 16+ messages in thread From: Dmitry Gutov @ 2021-01-04 17:10 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel, mardani29 On 04.01.2021 17:52, Eli Zaretskii wrote: >> The changes you are referring to are both in the test code. There's >> nothing inherently wrong in using concat in such code, as long as we >> test what we want to test. > > There's nothing wrong with that, sure, but you risk false failures due > to issues like the one here. However, if you are okay with that, I > don't mind. It just was strange to see such code. It's a matter of the interpretation. For one, if I saw this behavior with GNU find (which is installed on my machine), I'd want to fix it rather than just update the test. No harm in doing that just for BSD find either. >> If you actually meant to suggest an alternative comparison strategy >> (like in Daniel's latest patch), then I think the test becomes >> unnecessarily lax. After all, we want to also check how the string >> "looks" (because the user will see it verbatim), not just what file it >> points at. > > I don't think you can reliably know how the file name will be > formatted by a particular variety of 'find', not IME. But I'm not > going to argue about this. I think we can try. The fact that we've only seen such a report now is a good indicator. So, Daniel, how does the patch below work for you? It should also fix the same behavior in dired-do-find-regexp. diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el index d417382c0d..a87f054146 100644 --- a/lisp/progmodes/project.el +++ b/lisp/progmodes/project.el @@ -291,7 +291,8 @@ project--files-in-directory (localdir (file-local-name (expand-file-name dir))) (command (format "%s %s %s -type f %s -print0" find-program - localdir + ;; Omit the / for BSD find's sake. + (directory-file-name localdir) (xref--find-ignores-arguments ignores localdir) (if files (concat (shell-quote-argument "(") diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el index d2b5acd555..32efc2807c 100644 --- a/lisp/progmodes/xref.el +++ b/lisp/progmodes/xref.el @@ -1374,7 +1374,9 @@ xref-matches-in-directory ;; do that reliably enough, without creating false negatives? (command (xref--rgrep-command (xref--regexp-to-extended regexp) files - (file-local-name (expand-file-name dir)) + ;; Omit the / for BSD find's sake. + (directory-file-name + (file-local-name (expand-file-name dir))) ignores)) (def default-directory) (buf (get-buffer-create " *xref-grep*")) ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 17:10 ` Dmitry Gutov @ 2021-01-04 22:25 ` Daniel Martín 2021-01-06 1:38 ` Dmitry Gutov 0 siblings, 1 reply; 16+ messages in thread From: Daniel Martín @ 2021-01-04 22:25 UTC (permalink / raw) To: Dmitry Gutov; +Cc: Eli Zaretskii, emacs-devel Dmitry Gutov <dgutov@yandex.ru> writes: > > So, Daniel, how does the patch below work for you? It should also fix > the same behavior in dired-do-find-regexp. Thanks, it fixes the test failures for me. However, I still see the extra '/' if I follow these steps: - make TAGS - emacs -Q - Visit src/xdisp.c - M-? in redisplay_internal Perhaps there are more places that need that patch. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 22:25 ` Daniel Martín @ 2021-01-06 1:38 ` Dmitry Gutov 2021-01-06 9:41 ` Daniel Martín 0 siblings, 1 reply; 16+ messages in thread From: Dmitry Gutov @ 2021-01-06 1:38 UTC (permalink / raw) To: Daniel Martín; +Cc: Eli Zaretskii, emacs-devel [-- Attachment #1: Type: text/plain, Size: 1173 bytes --] On 05.01.2021 00:25, Daniel Martín wrote: > Thanks, it fixes the test failures for me. However, I still see the > extra '/' if I follow these steps: > > - make TAGS > - emacs -Q > - Visit src/xdisp.c > - M-? in redisplay_internal > > Perhaps there are more places that need that patch. Thanks, that's the third place, and probably the final one. In the meantime, plot thickens: as pointed out privately by Davis Herring, using 'find' on a directory without a trailing slash will fail if the directory is a symlink (it will only list the symlink itself). At this point we could double down on this approach and use the '-H' argument (though grep-find-template doesn't make this easy), or return to your original patch. Or yet alternatively, paper over this inside the xref-location-group defmethod. The two alternatives attached. So I guess I should ask: was the particular behavior annoying by itself, or is it only a problem because of the failing test? I also wonder whether all macOS users see this, or whether it's maybe fixed in some latest version of BSD find. Because it's apparently (arguably) a bug: https://unix.stackexchange.com/a/320440/166215 [-- Attachment #2: find-no-doubleslash-h.diff --] [-- Type: text/x-patch, Size: 2953 bytes --] diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el index 5f9a3fa352..21f075041c 100644 --- a/lisp/cedet/semantic/symref/grep.el +++ b/lisp/cedet/semantic/symref/grep.el @@ -119,12 +119,15 @@ semantic-symref-grep-use-template ;; it to the user. By contrast, here we don't show ;; the output, and the SGR escapes get in the way ;; of parsing the output. - (replace-regexp-in-string "--color=always" "" - grep-find-template t t) + (replace-regexp-in-string + "<D>" "-H <D>" + (replace-regexp-in-string "--color=always" "" + grep-find-template t t) + t t) grep-find-template) pattern filepattern - rootdir))) + (directory-file-name rootdir)))) cmd)) (defcustom semantic-symref-grep-shell shell-file-name diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el index d417382c0d..04546d1e4d 100644 --- a/lisp/progmodes/project.el +++ b/lisp/progmodes/project.el @@ -289,9 +289,9 @@ project--files-in-directory ;; expanded and not left for the shell command ;; to interpret. (localdir (file-local-name (expand-file-name dir))) - (command (format "%s %s %s -type f %s -print0" + (command (format "%s -H %s %s -type f %s -print0" find-program - localdir + (directory-file-name localdir) (xref--find-ignores-arguments ignores localdir) (if files (concat (shell-quote-argument "(") diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el index d2b5acd555..13fc2f09c7 100644 --- a/lisp/progmodes/xref.el +++ b/lisp/progmodes/xref.el @@ -1374,7 +1374,8 @@ xref-matches-in-directory ;; do that reliably enough, without creating false negatives? (command (xref--rgrep-command (xref--regexp-to-extended regexp) files - (file-local-name (expand-file-name dir)) + (directory-file-name + (file-local-name (expand-file-name dir))) ignores)) (def default-directory) (buf (get-buffer-create " *xref-grep*")) @@ -1543,7 +1544,10 @@ xref--rgrep-command ;; `shell-quote-argument' quotes the tilde as well. (cl-assert (not (string-match-p "\\`~" dir))) (grep-expand-template - grep-find-template + (replace-regexp-in-string + "<D>" "-H <D>" + grep-find-template + t t) regexp (concat (shell-quote-argument "(") " " find-name-arg " " [-- Attachment #3: find-no-doubleslash-postprocess.diff --] [-- Type: text/x-patch, Size: 3355 bytes --] diff --git a/lisp/cedet/semantic/symref/grep.el b/lisp/cedet/semantic/symref/grep.el index 5f9a3fa352..9f0ac38ec7 100644 --- a/lisp/cedet/semantic/symref/grep.el +++ b/lisp/cedet/semantic/symref/grep.el @@ -168,7 +168,8 @@ semantic-symref-perform-search (erase-buffer) (setq default-directory rootdir) (let ((cmd (semantic-symref-grep-use-template - (file-local-name rootdir) filepattern grepflags greppat))) + (file-name-as-directory (file-local-name rootdir)) + filepattern grepflags greppat))) (process-file semantic-symref-grep-shell nil b nil shell-command-switch cmd))) (setq ans (semantic-symref-parse-tool-output tool b)) diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el index d417382c0d..f91246a885 100644 --- a/lisp/progmodes/project.el +++ b/lisp/progmodes/project.el @@ -291,7 +291,7 @@ project--files-in-directory (localdir (file-local-name (expand-file-name dir))) (command (format "%s %s %s -type f %s -print0" find-program - localdir + (file-name-as-directory localdir) (xref--find-ignores-arguments ignores localdir) (if files (concat (shell-quote-argument "(") diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el index d2b5acd555..bacc5e6f1c 100644 --- a/lisp/progmodes/xref.el +++ b/lisp/progmodes/xref.el @@ -161,11 +161,13 @@ xref--project-root-memo "Cons mapping `default-directory' value to the search root.") (cl-defmethod xref-location-group ((l xref-file-location)) + (let ((file (oref l file))) + (unless (eq xref-file-name-display 'abs) + (setq file (replace-regexp-in-string "//" "/" file t t))) (cl-ecase xref-file-name-display - (abs - (oref l file)) + (abs file) (nondirectory - (file-name-nondirectory (oref l file))) + (file-name-nondirectory file)) (project-relative (unless (and xref--project-root-memo (equal (car xref--project-root-memo) @@ -176,12 +178,12 @@ xref-location-group (let ((pr (project-current))) (and pr (xref--project-root pr))))) (and root (expand-file-name root)))))) - (let ((file (oref l file)) + (let ((file file) (search-root (cdr xref--project-root-memo))) (if (and search-root (string-prefix-p search-root file)) (substring file (length search-root)) - file))))) + file)))))) (defclass xref-buffer-location (xref-location) ((buffer :type buffer :initarg :buffer) @@ -1374,7 +1376,8 @@ xref-matches-in-directory ;; do that reliably enough, without creating false negatives? (command (xref--rgrep-command (xref--regexp-to-extended regexp) files - (file-local-name (expand-file-name dir)) + (file-name-as-directory + (file-local-name (expand-file-name dir))) ignores)) (def default-directory) (buf (get-buffer-create " *xref-grep*")) ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-06 1:38 ` Dmitry Gutov @ 2021-01-06 9:41 ` Daniel Martín 2021-01-06 17:17 ` Dmitry Gutov 0 siblings, 1 reply; 16+ messages in thread From: Daniel Martín @ 2021-01-06 9:41 UTC (permalink / raw) To: Dmitry Gutov; +Cc: Eli Zaretskii, emacs-devel Dmitry Gutov <dgutov@yandex.ru> writes: > > In the meantime, plot thickens: as pointed out privately by Davis > Herring, using 'find' on a directory without a trailing slash will > fail if the directory is a symlink (it will only list the symlink > itself). > > At this point we could double down on this approach and use the '-H' > argument (though grep-find-template doesn't make this easy), or return > to your original patch. Or yet alternatively, paper over this inside > the xref-location-group defmethod. The two alternatives attached. > > So I guess I should ask: was the particular behavior annoying by > itself, or is it only a problem because of the failing test? > > I also wonder whether all macOS users see this, or whether it's maybe > fixed in some latest version of BSD find. Because it's apparently > (arguably) a bug: https://unix.stackexchange.com/a/320440/166215 I think the safest and simplest route is to just "fix" the tests so they pass in all systems. We shouldn't risk introducing regressions just to work around what POSIX says could be a 'find' bug in some BSD systems (and maybe only Apple systems, I don't have any other BSD system to test with). It only causes a slight cosmetic problem, which should be fixed whenever they fix the bug, without needing any code change from our side. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-06 9:41 ` Daniel Martín @ 2021-01-06 17:17 ` Dmitry Gutov 2021-01-06 18:48 ` Stefan Monnier 0 siblings, 1 reply; 16+ messages in thread From: Dmitry Gutov @ 2021-01-06 17:17 UTC (permalink / raw) To: Daniel Martín; +Cc: Eli Zaretskii, emacs-devel On 06.01.2021 11:41, Daniel Martín wrote: >> So I guess I should ask: was the particular behavior annoying by >> itself, or is it only a problem because of the failing test? >> >> I also wonder whether all macOS users see this, or whether it's maybe >> fixed in some latest version of BSD find. Because it's apparently >> (arguably) a bug: https://unix.stackexchange.com/a/320440/166215 > > I think the safest and simplest route is to just "fix" the tests so they > pass in all systems. We shouldn't risk introducing regressions just to > work around what POSIX says could be a 'find' bug in some BSD systems > (and maybe only Apple systems, I don't have any other BSD system to test > with). Thanks, that makes sense. We do try to work around problems in widely installed software, but so far it seems Apple-only. I've tried a few major versions of FreeBSD in a VM, going back to 11.4, and none have exhibited the problem. Guess we'll do the fix for the symlinks problem instead (the easy way), and then wait and see whether more reports come complaining about the double-slash issue. > It only causes a slight cosmetic problem, which should be fixed whenever > they fix the bug, without needing any code change from our side. AFAIK Apple tries very hard to avoid updating to the latest versions of Unixy tools. Luckily, there are now several projects that let users install fresh GNU versions of said tools (and other command-line programs). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-06 17:17 ` Dmitry Gutov @ 2021-01-06 18:48 ` Stefan Monnier 2021-01-06 21:17 ` Dmitry Gutov 0 siblings, 1 reply; 16+ messages in thread From: Stefan Monnier @ 2021-01-06 18:48 UTC (permalink / raw) To: Dmitry Gutov; +Cc: Eli Zaretskii, emacs-devel, Daniel Martín >> It only causes a slight cosmetic problem, which should be fixed whenever >> they fix the bug, without needing any code change from our side. > AFAIK Apple tries very hard to avoid updating to the latest versions of > Unixy tools. AFAIK what they avoid like the plague is GPLv3, so they stick to the last release of GNU tools that used GPLv2 (and/or switch to tools using non-copyleft licenses, like their switch from bash to zsh). Stefan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-06 18:48 ` Stefan Monnier @ 2021-01-06 21:17 ` Dmitry Gutov 0 siblings, 0 replies; 16+ messages in thread From: Dmitry Gutov @ 2021-01-06 21:17 UTC (permalink / raw) To: Stefan Monnier; +Cc: Eli Zaretskii, emacs-devel, Daniel Martín On 06.01.2021 20:48, Stefan Monnier wrote: > AFAIK what they avoid like the plague is GPLv3, so they stick to the > last release of GNU tools that used GPLv2 (and/or switch to tools using > non-copyleft licenses, like their switch from bash to zsh). Right, but if they are using a BSD find, they could upgrade it to some recent version (like ones found in FreeBSD 11+). Or switch to it. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-03 17:16 ` [PATCH] Fix some failing tests in BSD systems Daniel Martín 2021-01-03 17:35 ` Eli Zaretskii @ 2021-01-04 1:43 ` Dmitry Gutov 2021-01-04 3:48 ` Stefan Monnier 1 sibling, 1 reply; 16+ messages in thread From: Dmitry Gutov @ 2021-01-04 1:43 UTC (permalink / raw) To: Daniel Martín, emacs-devel Hi Daniel, On 03.01.2021 19:16, Daniel Martín wrote: > > A couple of tests are failing for me on master and a BSD system (macOS): > > 2 unexpected results: > FAILED xref--xref-file-name-display-is-abs > FAILED xref--xref-file-name-display-is-relative-to-project-root > > An extract of a test failure is > > (list-elt 0 (arrays-of-different-length 25 24 > "xref-resources//file1.txt" "xref-resources/file1.txt" first-mismatch-at > 15)) > > The reason for the failure is that BSD 'find' sometimes outputs paths > with an extra '/', for example: > > find /path/emacs/test/lisp/progmodes/xref-resources/ -type f \( -iname \* \) > /path/emacs/test/lisp/progmodes/xref-resources//file2.txt > /path/emacs/test/lisp/progmodes/xref-resources//file1.txt > > (Note the trailing '/' in the first argument to 'find'.) > > As I think this differs from GNU find but it's still a valid POSIX path, > I decided to make the tests more flexible. I don't mind the proposed change to the tests, but if this is the only situation when the BSD find behaves differently, we could try to avoid it like this: diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el index d2b5acd555..019c9a35bc 100644 --- a/lisp/progmodes/xref.el +++ b/lisp/progmodes/xref.el @@ -1374,7 +1374,8 @@ xref-matches-in-directory ;; do that reliably enough, without creating false negatives? (command (xref--rgrep-command (xref--regexp-to-extended regexp) files - (file-local-name (expand-file-name dir)) + (directory-file-name + (file-local-name (expand-file-name dir))) ignores)) (def default-directory) (buf (get-buffer-create " *xref-grep*")) ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Fix some failing tests in BSD systems 2021-01-04 1:43 ` Dmitry Gutov @ 2021-01-04 3:48 ` Stefan Monnier 0 siblings, 0 replies; 16+ messages in thread From: Stefan Monnier @ 2021-01-04 3:48 UTC (permalink / raw) To: Dmitry Gutov; +Cc: emacs-devel, Daniel Martín > I don't mind the proposed change to the tests, but if this is the only > situation when the BSD find behaves differently, we could try to avoid it > like this: It seems the most "frugal" fix, indeed. But it deserves a comment explaining the subtle choice. Stefan ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-01-06 21:17 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <m1sg7i7xfl.fsf.ref@yahoo.es> 2021-01-03 17:16 ` [PATCH] Fix some failing tests in BSD systems Daniel Martín 2021-01-03 17:35 ` Eli Zaretskii 2021-01-03 21:08 ` Daniel Martín 2021-01-04 1:41 ` Dmitry Gutov 2021-01-04 3:30 ` Eli Zaretskii 2021-01-04 13:10 ` Dmitry Gutov 2021-01-04 15:52 ` Eli Zaretskii 2021-01-04 17:10 ` Dmitry Gutov 2021-01-04 22:25 ` Daniel Martín 2021-01-06 1:38 ` Dmitry Gutov 2021-01-06 9:41 ` Daniel Martín 2021-01-06 17:17 ` Dmitry Gutov 2021-01-06 18:48 ` Stefan Monnier 2021-01-06 21:17 ` Dmitry Gutov 2021-01-04 1:43 ` Dmitry Gutov 2021-01-04 3:48 ` Stefan Monnier
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.